svn commit: r252313 - head/sys/kern

[Mikolaj Golub]

On Fri, Jun 28, 2013 at 09:44:30AM +0300, Konstantin Belousov wrote:
> On Fri, Jun 28, 2013 at 03:03:46AM +0200, Mateusz Guzik wrote:
> > On Thu, Jun 27, 2013 at 07:14:04PM +0000, Mikolaj Golub wrote:
> > > Author: trociny
> > > Date: Thu Jun 27 19:14:03 2013
> > > New Revision: 252313
> > > URL: http://svnweb.freebsd.org/changeset/base/252313
> > > 
> > > Log:
> > >   To avoid LOR, always drop the filedesc lock before exporting fd to sbuf.
> > >   
> > >   Reviewed by:	kib
> > >   MFC after:	3 days
> > > 
> > > Modified:
> > >   head/sys/kern/kern_descrip.c
> > > 
> > > Modified: head/sys/kern/kern_descrip.c
> > > ==============================================================================
> > > --- head/sys/kern/kern_descrip.c	Thu Jun 27 18:59:07 2013	(r252312)
> > > +++ head/sys/kern/kern_descrip.c	Thu Jun 27 19:14:03 2013	(r252313)
> > > @@ -3427,12 +3427,10 @@ kern_proc_filedesc_out(struct proc *p,  
> > >  		 * re-validate and re-evaluate its properties when
> > >  		 * the loop continues.
> > >  		 */
> > > -		if (type == KF_TYPE_VNODE || type == KF_TYPE_FIFO)
> > > -			FILEDESC_SUNLOCK(fdp);
> > > +		FILEDESC_SUNLOCK(fdp);
> > >  		error = export_fd_to_sb(data, type, i, fflags, refcnt,
> > >  		    offset, fd_cap_rights, kif, sb, &remainder);
> > > -		if (type == KF_TYPE_VNODE || type == KF_TYPE_FIFO)
> > > -			FILEDESC_SLOCK(fdp);
> > > +		FILEDESC_SLOCK(fdp);
> > >  		if (error)
> > >  			break;
> > >  	}
> > 
> > Is this really ok? What prevents given fd from going away during
> > export_fd_to_sb execution? Both DTYPE_VNODE and DTYPE_FIFO pass down
> > a vrefed vnode so these are safe. But for example DTYPE_SOCKET goes with
> > fp->f_data, which can go away in the meantime (or I'm misreading the code).
> > 

Thanks!

> > I suggest obtainng ref to fp and passing it down in all cases.
> 
> Oops, I am sorry for missed this. But, I do not actually like the
> idea of referencing the fd.  It de-facto means that the process calling
> the sysctl duped the descriptor, potentially causing the close to be
> performed in the sysctl context on dereference.
> 
> Ideal solution would be to drop the filedesc lock between processing
> of the filedescriptors and draining sbuf while lock is dropped.

You mean something like below? (not well tested yet)

-- 
Mikolaj Golub
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kern_proc_filedesc_out.lor.2.patch
Type: text/x-diff
Size: 4742 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20130628/86ab288b/attachment.patch>