Race conditions

[Konstantin Belousov]

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.