Date: Wed, 19 Aug 2015 11:48:34 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: John Baldwin <john@araratriver.co> Cc: freebsd-drivers@freebsd.org, Leonardo Fogel <leonardofogel@yahoo.com.br>, "'Konstantin Belousov'" <kib@freebsd.org> Subject: Re: Race conditions Message-ID: <20150819084834.GM2072@kib.kiev.ua> In-Reply-To: <10064388.a9lbzVPoX7@ralph.baldwin.cx> References: <1439923294.98963.YahooMailBasic@web120801.mail.ne1.yahoo.com> <10064388.a9lbzVPoX7@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Aug 18, 2015 at 07:30:48PM -0700, John Baldwin wrote: > On Tuesday, August 18, 2015 11:41:34 AM Leonardo Fogel wrote: > > Hi. > > The following code is an exerpt from the FreeBSD Architecture Handbook, chapter 11.1.1. Sample Driver Source. I've included labels i1, i2, i3. > > > > int > > mypci_open(struct cdev *dev, int oflags, int devtype, struct thread *td) > > { > > struct mypci_softc *sc; > > > > /* Look up our softc. */ > > i1: sc = dev->si_drv1; if (sc == NULL) return (ENXIO); The new cdev was allocated with M_ZERO flag, so you can rely on the fact that uninitalized fields are zeroed. > > device_printf(sc->my_dev, "Opened successfully.\n"); > > return (0); > > } > > > > static int > > mypci_attach(device_t dev) > > { > > struct mypci_softc *sc; > > ... > > i2: sc->my_cdev = make_dev(&mypci_cdevsw, device_get_unit(dev), > > UID_ROOT, GID_WHEEL, 0600, "mypci%u", device_get_unit(dev)); > > i3: sc->my_cdev->si_drv1 = sc; > > printf("Mypci device loaded.\n"); > > return (0); > > } > > > > > > As I understand it, as soon as instruction at label i2 completes, there is a rare possibility that another thread opens the device file and executes the instruction at i1, before the instruction at i3 is executed. Is it correct? How could one fix it? > > It is a race, yes. I know there is a MAKEDEV_REF flag that can be passed to > make_dev_p() and make_dev_credf() that can hold a reference on the returned > cdev (so it can't be immediately deleted), but I don't know that that helps > close the race you reference. No, MAKEDEV_REF is about calling dev_ref() early enough so that the dev_clone handlers could safely access cdev that was just created (otherwise other thread might enter devfs_populate_loop() in parallel and unref :( ). MAKEDEV_REF has nothing to do with driver-managed fields initialization. > > I think I would probably prefer we add some sort of wrapper for make_dev > that accepts the si_drv1 value (and possibly other values) as an argument to > close this. I'm cc'ing kib@ to see if he has any suggestions or better ideas. Yes, this is a known issue in the cdev KPI, but of very low importance. I agree that a change to cdev KPI is due. One of the existing issues is that it is already bloated with make_dev_credf make_dev_cred make_dev_p make_dev all grown organically to plug this and that uglyness in the KPI. I wanted to combine all non-naming parameters to make_dev* into some structure, so that the final function to create cdev is like int make_dev_uber(struct cdev **res, struct make_dev_args *args, const char *fmt, ...); struct make_dev_args { struct cdevsw *csw; int flags; struct ucred *cred; ... int si_drv0; void *si_drv1, *si_drv2; <- eventually }; and a helper to do initialization of the structure. But as I said above, it is very low priority and I want to gather more outstanding issues with the KPI before making any decisions there.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150819084834.GM2072>