Date: Wed, 20 Feb 2013 07:53:38 -0700 From: Jamie Gritton <jamie@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: fs@FreeBSD.org Subject: Re: mount/kldload race Message-ID: <5124E372.1000009@FreeBSD.org> In-Reply-To: <20130220054309.GD2598@kib.kiev.ua> References: <51244A13.8030907@FreeBSD.org> <20130220054309.GD2598@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5124E372.1000009>