On Thu, Apr 01, 2021 at 08:25:11AM -0300, Jason Gunthorpe wrote: > On Thu, Apr 01, 2021 at 12:00:39PM +0300, Dan Carpenter wrote: > > Hi Keith, > > > > I've been trying to figure out ways Smatch can check for device managed > > resources. Like adding rules that if we call dev_set_name(&foo->bar) > > then it's device managaged and if there is a kfree(foo) without calling > > device_put(&foo->bar) then that's a resource leak. > > It seems to be working from what I can see This check is actually more simple, and older. It just looks for device_register(dev); ... kfree(dev); I've written your proposed check of: device_register(&foo->dev); ... kfree(foo); // warning missing device_put(&foo->dev); But I just did that earler today and it will probably take a couple iterations to work out the kinks. Plus I'm off for a small vacation so it will be a week before I have the results from that. > > Also I wasn't able to convince myself that any locking around > node->cache_attrs exist.. > > > Of course one of the rules is that if you call device_register(dev) then > > you can't kfree(dev), it has to released with device_put(dev) and that's > > true even if the register fails. But this code here feels very > > intentional so maybe there is an exception to the rule? > > There is no exception. Open coding this: > > > 282 free_name: > > 283 kfree_const(dev->kobj.name); > > To avoid leaking memory from dev_set_name is a straight up layering > violation, WTF? > > node_cacheinfo_release() is just kfree(), so there is no need. > Instead (please feel free to send this Dan): Sure, I can send this (tomorrow). > > diff --git a/drivers/base/node.c b/drivers/base/node.c > index f449dbb2c74666..89c28952863977 100644 > --- a/drivers/base/node.c > +++ b/drivers/base/node.c > @@ -319,25 +319,24 @@ void node_add_cache(unsigned int nid, struct node_cache_attrs *cache_attrs) > return; > > dev = &info->dev; > + device_initialize(dev) > dev->parent = node->cache_dev; > dev->release = node_cacheinfo_release; > dev->groups = cache_groups; > if (dev_set_name(dev, "index%d", cache_attrs->level)) Is calling dev_set_name() without doing a device_initialize() a bug? I could write a check for that. regards, dan carpenter
