mount/kldload race

[Konstantin Belousov]

On Wed, Feb 20, 2013 at 07:53:38AM -0700, Jamie Gritton wrote:
> On 02/19/13 22:43, Konstantin Belousov wrote:
> > On Tue, Feb 19, 2013 at 08:59:15PM -0700, Jamie Gritton wrote:
> >> Perhaps most people don't try to mount a bunch of filesystems at the
> >> same time, at least not those that depend on kernel modules. But it
> >> turns out that's going to be a pretty common situation with jails and
> >> nullfs. And I found that when attempting such a feat will cause most of
> >> these simultaneous mounts to fail with ENODEV.
> >>
> >> It turns out that the problem is a race in vfs_byname_kld(). First it'll
> >> see if the fstype is loaded, and if it isn't then it will load the
> >> module. But if the module is loaded by a different process between those
> >> two points, the resulting EEXIST from kern_kldload() will make
> >> vfs_byname_kld() error out.
> >>
> >> The fix is pretty simple: don't treat EEXIST as an error. By going on,
> >> and rechecking for the fstype, the filesystem can be mounted while still
> >> allowing any "real" error to be caught. I'm including a small patch that
> >> will accomplish this, and I'd appreciate a quick look by anyone who's
> >> familiar with this part of things before I commit it.
> >>
> >> - Jamie
> >
> >> Index: sys/kern/vfs_init.c
> >> ===================================================================
> >> --- sys/kern/vfs_init.c	(revision 247000)
> >> +++ sys/kern/vfs_init.c	(working copy)
> >> @@ -130,13 +130,18 @@
> >>
> >>   	/* Try to load the respective module. */
> >>   	*error = kern_kldload(td, fstype,&fileid);
> >> +	if (*error == EEXIST) {
> >> +		*error = 0;
> >> +		fileid = 0;
> > Why do you clear fileid ? Is this to prevent an attempt to kldunload()
> > the module which was not loaded by the current thread ?
> >
> > If yes, I would suggest to use the separate flag to track this,
> > which is cleared on EEXIST error. IMHO it is cleaner and less puzzling.
> 
> Yes, that's why.  As a side note, I clear *error ostensibly for the sake 
> of the callers, but it turns out none of the callers actually look at 
> the returned error.
> 
> Here's a new patch with an added flag:
I have no further comments, looks good.

> 
> 
> Index: sys/kern/vfs_init.c
> ===================================================================
> --- sys/kern/vfs_init.c	(revision 247000)
> +++ sys/kern/vfs_init.c	(working copy)
> @@ -122,7 +122,7 @@
>   vfs_byname_kld(const char *fstype, struct thread *td, int *error)
>   {
>   	struct vfsconf *vfsp;
> -	int fileid;
> +	int fileid, loaded;
> 
>   	vfsp = vfs_byname(fstype);
>   	if (vfsp != NULL)
> @@ -130,13 +130,17 @@
> 
>   	/* Try to load the respective module. */
>   	*error = kern_kldload(td, fstype, &fileid);
> +	loaded = (*error == 0);
> +	if (*error == EEXIST)
> +		*error = 0;
>   	if (*error)
>   		return (NULL);
> 
>   	/* Look up again to see if the VFS was loaded. */
>   	vfsp = vfs_byname(fstype);
>   	if (vfsp == NULL) {
> -		(void)kern_kldunload(td, fileid, LINKER_UNLOAD_FORCE);
> +		if (loaded)
> +			(void)kern_kldunload(td, fileid, LINKER_UNLOAD_FORCE);
>   		*error = ENODEV;
>   		return (NULL);
>   	}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20130220/3a6473e9/attachment.sig>