On Thu, Mar 25, 2021 at 09:52:05AM +0300, Dan Carpenter wrote:
> On Wed, Mar 24, 2021 at 08:35:25PM -0300, Jason Gunthorpe wrote:
> > On Wed, Mar 24, 2021 at 10:52:52PM +0300, Dan Carpenter wrote:
> > > On Wed, Mar 24, 2021 at 01:52:46PM -0300, Jason Gunthorpe wrote:
> > > > On Wed, Mar 24, 2021 at 09:13:35AM -0700, Dan Williams wrote:
> > > >
> > > > > Which is just:
> > > > >
> > > > > device_initialize()
> > > > > dev_set_name()
> > > > >
> > > > > ...then the name is set as early as the device is ready to filled in
> > > > > with other details. Just checking for dev_set_name() failures does not
> > > > > move the api forward in my opinion.
> > > >
> > > > This doesn't work either as the release function must be set after
> > > > initialize but before dev_set_name(), otherwise we both can't and must
> > > > call put_device() after something like this fails.
> > > >
> > > > I can't see an option other than bite the bullet and fix things.
> > > >
> > > > A static tool to look for these special lifetime rules around the
> > > > driver core would be nice.
> > >
> > > If y'all are specific enough about what you want, then I can write the
> > > check for you. What I really want is some buggy sample code and the
> > > warning you want me to print. I kind of vaguely know that devm_ life
> > > time rules are tricky but I don't know the details.
> >
> > This is driver core rules.
> >
> > The setup is:
> >
> > struct foo_device
> > {
> > struct device dev;
> > }
> >
> > struct foo_device *fdev = kzalloc(sizeo(*fdev), GFP_KERNEL);
> >
> > Then in each of these situations:
> >
> > device_initialize(&fdev->dev);
> > // WARNING initialized struct device's must be destroyed with put_device()
> > kfree(fdev);
> >
>
> This email is perfect! Exactly what I want. My one question would be
> what happens if we don't call put_device() in this first example?
*Usually* nothing bad, but it is wrong coding and against the API
contract. In more complicated situations it becomes impossible to
really tell if it is OK.
The rule is once reference counting starts you have to use reference
counting for free.
> The laziest thing would be to just add them to check_unwind.c:
>
> { "device_initialize", ALLOC, 0, "$" },
> { "dev_set_name", ALLOC, 0, "$" },
> { "device_register", ALLOC, 0, "$" },
> { "put_device", RELEASE, 0, "$" },
>
> The check_unwind.c file assumes that every function cleans up after
> itself on error. It doesn't look for the kfree(fdev). I could make
> kfree() the rule if you want. I tested it on one file to see if it
> generated a warning and it does.
>
> net/atm/atm_sysfs.c:167 atm_register_sysfs() warn: '&adev->class_dev' not released on lines: 153,167.
I don't know much about check_unwind.c, but it is similiar to
kalloc/kfree rules? ie the kfree could be in some other function
If you want to be highly precise the control flow we are searching for
really is kfree following any of the above functions (cross function too)
> The line 153 is a real bug
Yes
> but line 167 calls device_del(). The
> comments device_del() say "NOTE: this should be called manually _iff_
> device_add() was also called manually." which suggests that this is a
> different sort of bug... Should I add device_del() optional release
> function? I have device_unregister() there already.
Yes, it is wrong too, it should be device_unregister, except that
would kfree the device and the caller isn't prepared for that.
The flow here is:
struct atm_dev *atm_dev_register(const char *type, struct device *parent,
dev = __alloc_atm_dev(type);
if (atm_register_sysfs(dev, parent) < 0) {
goto out_fail;
out_fail:
kfree(dev);
atm_register_sysfs():
err = device_register(&dev->cdev);
if (err < 0)
return err;
So we kfree after doing dev_set_name which leaks memory.
The whole thing is the kind of nonsense I hate - it is trying really
hard to use device_regsiter and it *really* need to be coded with the
device_initiailze/add split, device_initalize should be called in
atm_dev_register() directly after registration.
Jason
