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?
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.
The line 153 is a real bug, 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.
{ "device_unregister", RELEASE, 0, "$" },
{ "device_del", RELEASE, 0, "$" },
Do we care about nesting?
Anyway, I'm going to pull these out of check_unwind.c into their own
file so that I can make the warning messages a bit better.
regards,
dan carpenter
