mount/kldload race

[Jamie Gritton]

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:


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);
  	}