Date: Wed, 20 Feb 2013 17:37:05 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Jamie Gritton <jamie@FreeBSD.org> Cc: fs@FreeBSD.org Subject: Re: mount/kldload race Message-ID: <20130220153705.GE2598@kib.kiev.ua> In-Reply-To: <5124E372.1000009@FreeBSD.org> References: <51244A13.8030907@FreeBSD.org> <20130220054309.GD2598@kib.kiev.ua> <5124E372.1000009@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--UOi+gfmBpEZPw9cU
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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 tho=
se
> >> 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 sti=
ll
> >> allowing any "real" error to be caught. I'm including a small patch th=
at
> >> 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
> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> --- 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 =3D kern_kldload(td, fstype,&fileid);
> >> + if (*error =3D=3D EEXIST) {
> >> + *error =3D 0;
> >> + fileid =3D 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.
>=20
> Yes, that's why. As a side note, I clear *error ostensibly for the sake=
=20
> of the callers, but it turns out none of the callers actually look at=20
> the returned error.
>=20
> Here's a new patch with an added flag:
I have no further comments, looks good.
>=20
>=20
> Index: sys/kern/vfs_init.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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;
>=20
> vfsp =3D vfs_byname(fstype);
> if (vfsp !=3D NULL)
> @@ -130,13 +130,17 @@
>=20
> /* Try to load the respective module. */
> *error =3D kern_kldload(td, fstype, &fileid);
> + loaded =3D (*error =3D=3D 0);
> + if (*error =3D=3D EEXIST)
> + *error =3D 0;
> if (*error)
> return (NULL);
>=20
> /* Look up again to see if the VFS was loaded. */
> vfsp =3D vfs_byname(fstype);
> if (vfsp =3D=3D NULL) {
> - (void)kern_kldunload(td, fileid, LINKER_UNLOAD_FORCE);
> + if (loaded)
> + (void)kern_kldunload(td, fileid, LINKER_UNLOAD_FORCE);
> *error =3D ENODEV;
> return (NULL);
> }
--UOi+gfmBpEZPw9cU
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iQIcBAEBAgAGBQJRJO2gAAoJEJDCuSvBvK1Bj44P/3XSMKbo4w/4W3Gf7MnCcn01
NOkmXv36TthOxkXUEOWldzKUQzYuvG4lbQ8m8w+nrlP5C5wVFY2a7jWAZBHLEs96
m/H35R5Dke3a4UEd1Q7lb47LhIhR65hmzyBYCP2ylwLKdAI4kuWxmoFPV62QFIqt
csMomk2N7FtMB4F4ryMxHTA6ERzbjh5JE+kdR2KtDFXOdWeDa7no4NYnXjAcxACK
+ikfCHrvYfgsq3goUg12CSFbQth9naKSbRtSm5qXF07akjnwNc+mtrni4mxN/+YM
8kgehvojPIFkDzOw+tTK7EnRlTovXroV0VcDm0r0rAZFo/3eJxvh5nIqbyBpM4oO
tqTs1qGk1o4XA8lytAfP2UFUb9LOD2CIarjcre6Mj/tF5t0CThZLJEZvLRjxNZ0L
Cp46C/vf6m348UsP06gu74WibtGwVBlJthr+IXwLUZnNvsKrDZLrq4EXl7n1gxJ9
VYkeIYStpKklwj/6h0GHBAZhF9sbDUsqUPQ6VFnKz6EOQIxsZTIQtytWImLOTArZ
DHZRKta7AdtylDk8YHH83p90AMEduCBFWse/ZGv7+kcUbyehtfMm1QmTkdBwPZd4
WYKd/Gy5y35xgD+MWuTvly7aqss3dwHk581CQyfAjFYEkvcW8VZtLYDrgbrk9rfu
/vE7ZFtHooVRyr6mlsV2
=PTS+
-----END PGP SIGNATURE-----
--UOi+gfmBpEZPw9cU--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130220153705.GE2598>