Fwd: kobject_init and the zeroed-out-memory requirement

Fwd: kobject_init and the zeroed-out-memory requirement

[Jason Noakes]

I noticed that kobject_init() requres the kobject passed in to be
zeroed out fully first.

Many other *_init kernel routines (cdev_init, kref_init, mutex_init,
spin_lock_init, etc) do not have the same requirement - they work on
fully uninitialized memory.

Documentation/kobject.txt does not mention the requirement that the
memory be zero-initialized before it is passed to kobject_init.

I would like to submit a patch - which solution is preferred?

  (a) Update Documentation/kobject.txt and explicitly add the requirement
  (b) Modify kobject_init to zero out the memory itself like other
*_init routines
        (It already initializes most of its members - just not all of them)
  (c) Something else?

Your preference?

(Please CC: me on replies).

-- 
~~ Jason J. Noakes
~~ jjnoakes@gmail.com

Re: kobject_init and the zeroed-out-memory requirement

[Greg KH]

On Sun, Oct 05, 2014 at 04:02:14PM -0400, Jason Noakes wrote:
> I noticed that kobject_init() requres the kobject passed in to be zeroed out
> fully first.

That is because people were trying to reuse objects without destroying
them first.  So try to detect this and prevent the developer from doing
something "foolish" like this.

Is there any in-kernel code that does not properly zero out the memory
before calling kobject_init()?

> Many other *_init kernel routines (cdev_init, kref_init, mutex_init,
> spin_lock_init, etc) do not have the same requirement - they work on fully
> uninitialized memory.

They all do different things, you can't compare apples to oranges :)

> Documentation/kobject.txt does not mention the requirement that the memory be
> zero-initialized before it is passed to kobject_init.
> 
> I would like to submit a patch - which solution is preferred?
> 
>   (a) Update Documentation/kobject.txt and explicitly add the requirement
>   (b) Modify kobject_init to zero out the memory itself like other *_init
> routines
>         (It already initializes most of its members - just not all of them)

As mentioned above, this is not going to be ok, look at the third if
statement in kobject_init() for why not.

>   (c) Something else?

Add a line of text to the kerneldoc for kobject_init to mention this?

Or (a) is fine as well, if it makes you feel better, but if you do so,
just say that all memory for kobjects must be created with kzalloc() and
don't mention memset as that will cause people to try to reuse kobjects,
like they have in the past, and bad things will happen then.

hope this helps,

greg k-h

Re: kobject_init and the zeroed-out-memory requirement

[Jason Noakes]

> Is there any in-kernel code that does not properly zero out the memory
> before calling kobject_init()?

I'm not sure. I didn't find any, but I've seen it bite people writing
drivers more than once where I work, and the latest oops I just
debugged a few days ago prompted me to address the issue and at least
get the documentation updated.

>> Many other *_init kernel routines (cdev_init, kref_init, mutex_init,
>> spin_lock_init, etc) do not have the same requirement - they work on fully
>> uninitialized memory.
>
> They all do different things, you can't compare apples to oranges :)

Well, they are all named "_init", which to me implies that they
initialize the object/memory passed in, which is true in all of the
other cases I mentioned, but not for kobject_init. Perhaps it should
have been called kobject_setup or something. But I digress...

>> Documentation/kobject.txt does not mention the requirement that the memory be
>> zero-initialized before it is passed to kobject_init.
>
> Add a line of text to the kerneldoc for kobject_init to mention this?
>
> Or (a) is fine as well, if it makes you feel better, but if you do so,
> just say that all memory for kobjects must be created with kzalloc() and
> don't mention memset as that will cause people to try to reuse kobjects,
> like they have in the past, and bad things will happen then.

Here is a patch updating the documentation (kobject.txt and
kobject_init's usage documentation) so that the zeroed-out-memory
requirement is explicit.

Signed-off-by: Jason J. Noakes <jjnoakes@gmail.com>

diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt
index f87241d..1b38727 100644
--- a/Documentation/kobject.txt
+++ b/Documentation/kobject.txt
@@ -113,8 +113,14 @@ question.  That macro is subsequently invoked with:

 Initialization of kobjects

-Code which creates a kobject must, of course, initialize that object. Some
-of the internal fields are setup with a (mandatory) call to kobject_init():
+Code which creates a kobject must, of course, initialize that object.
+
+First, ensure the memory for the kobject is initialized to zero; this is
+typically accomplished by obtaining the memory for the kobject (and the
+structure it is embedded in) from a call to kzalloc().
+
+Then, some of the internal fields must be setup with a (mandatory) call
+to kobject_init():

     void kobject_init(struct kobject *kobj, struct kobj_type *ktype);

diff --git a/lib/kobject.c b/lib/kobject.c
index 58751bb..46fc865 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -307,6 +307,8 @@ EXPORT_SYMBOL(kobject_set_name);
  * This function will properly initialize a kobject such that it can then
  * be passed to the kobject_add() call.
  *
+ * The kobject must be initialized to zero prior to calling this function.
+ *
  * After this function is called, the kobject MUST be cleaned up by a call
  * to kobject_put(), not by a call to kfree directly to ensure that all of
  * the memory is cleaned up properly.

Re: kobject_init and the zeroed-out-memory requirement

[Greg KH]

On Sun, Oct 05, 2014 at 04:47:42PM -0400, Jason Noakes wrote:
> > Is there any in-kernel code that does not properly zero out the memory
> > before calling kobject_init()?
> 
> I'm not sure. I didn't find any, but I've seen it bite people writing
> drivers more than once where I work, and the latest oops I just
> debugged a few days ago prompted me to address the issue and at least
> get the documentation updated.

No driver should be working with "raw" kobjects.

> >> Many other *_init kernel routines (cdev_init, kref_init, mutex_init,
> >> spin_lock_init, etc) do not have the same requirement - they work on fully
> >> uninitialized memory.
> >
> > They all do different things, you can't compare apples to oranges :)
> 
> Well, they are all named "_init", which to me implies that they
> initialize the object/memory passed in, which is true in all of the
> other cases I mentioned, but not for kobject_init. Perhaps it should
> have been called kobject_setup or something. But I digress...

kobject_init() has been there for a very long time, and yes, we don't
always have the best naming scheme in the kernel, that comes from
evolution over the years.

> >> Documentation/kobject.txt does not mention the requirement that the memory be
> >> zero-initialized before it is passed to kobject_init.
> >
> > Add a line of text to the kerneldoc for kobject_init to mention this?
> >
> > Or (a) is fine as well, if it makes you feel better, but if you do so,
> > just say that all memory for kobjects must be created with kzalloc() and
> > don't mention memset as that will cause people to try to reuse kobjects,
> > like they have in the past, and bad things will happen then.
> 
> Here is a patch updating the documentation (kobject.txt and
> kobject_init's usage documentation) so that the zeroed-out-memory
> requirement is explicit.
> 
> Signed-off-by: Jason J. Noakes <jjnoakes@gmail.com>

Care to resend this in a format I can apply it in (i.e. without this
discussion above the patch forcing me to edit the changelog)?

thanks,

greg k-h

Re: kobject_init and the zeroed-out-memory requirement

[Jason Noakes]

> No driver should be working with "raw" kobjects.

I don't agree, but it's irrelevant. If the functions are exported and
documented, the documentation should be complete.

> kobject_init() has been there for a very long time, and yes, we don't
> always have the best naming scheme in the kernel, that comes from
> evolution over the years.

If you ever want to rename it, drop me a line. I'll submit the patch
for that too :)

> Care to resend this in a format I can apply it in (i.e. without this
> discussion above the patch forcing me to edit the changelog)?

Sure.

-- 
~~ Jason J. Noakes
~~ jjnoakes@gmail.com

Re: kobject_init and the zeroed-out-memory requirement

[Greg KH]

On Sun, Oct 05, 2014 at 06:13:05PM -0400, Jason Noakes wrote:
> > No driver should be working with "raw" kobjects.
> 
> I don't agree, but it's irrelevant.

Not at all.  I'd wager that if a driver is messing around with a "raw"
kobject, it is doing something seriously wrong.  Of course there are
exceptions, but those are very rare, and exceptions.  A driver should be
using the driver core, and the functions and objects provided there, and
provided by the bus it lives on.

So, have a pointer to some driver code that is calling
kobject_initialize()?  I'd love to see it.

thanks,

greg k-h

Re: kobject_init and the zeroed-out-memory requirement

[Guenter Roeck]

On Sun, Oct 05, 2014 at 04:24:57PM -0700, Greg KH wrote:
> On Sun, Oct 05, 2014 at 06:13:05PM -0400, Jason Noakes wrote:
> > > No driver should be working with "raw" kobjects.
> > 
> > I don't agree, but it's irrelevant.
> 
> Not at all.  I'd wager that if a driver is messing around with a "raw"
> kobject, it is doing something seriously wrong.  Of course there are
> exceptions, but those are very rare, and exceptions.  A driver should be
> using the driver core, and the functions and objects provided there, and
> provided by the bus it lives on.
> 
> So, have a pointer to some driver code that is calling
> kobject_initialize()?  I'd love to see it.
> 

Lots to choose from. In drivers/:

$ git grep kobject_init | wc
     65     289    4880
$ git grep kobject_init | grep -v base | wc
     62     277    4694
$ git grep kobject_init_and_add | grep -v base | wc
     47     232    3698
$ git grep kobject_add | grep -v base | wc
     20     111    1486

Guenter

Re: kobject_init and the zeroed-out-memory requirement

[Greg KH]

On Sun, Oct 05, 2014 at 07:09:50PM -0700, Guenter Roeck wrote:
> On Sun, Oct 05, 2014 at 04:24:57PM -0700, Greg KH wrote:
> > On Sun, Oct 05, 2014 at 06:13:05PM -0400, Jason Noakes wrote:
> > > > No driver should be working with "raw" kobjects.
> > > 
> > > I don't agree, but it's irrelevant.
> > 
> > Not at all.  I'd wager that if a driver is messing around with a "raw"
> > kobject, it is doing something seriously wrong.  Of course there are
> > exceptions, but those are very rare, and exceptions.  A driver should be
> > using the driver core, and the functions and objects provided there, and
> > provided by the bus it lives on.
> > 
> > So, have a pointer to some driver code that is calling
> > kobject_initialize()?  I'd love to see it.
> > 
> 
> Lots to choose from. In drivers/:
> 
> $ git grep kobject_init | wc
>      65     289    4880
> $ git grep kobject_init | grep -v base | wc
>      62     277    4694
> $ git grep kobject_init_and_add | grep -v base | wc
>      47     232    3698
> $ git grep kobject_add | grep -v base | wc
>      20     111    1486

Well, you hit firmware layer, and block devices, which have to deal with
kobjects directly at times.  But these files look "suspicious":
	edac/edac_device_sysfs.c
	edac/edac_pci_sysfs.c
	gpu/drm/ttm/ttm_bo.c
	gpu/drm/ttm/ttm_memory.c
	gpu/drm/ttm/ttm_page_alloc.c
	gpu/drm/ttm/ttm_page_alloc_dma.c
	infiniband/core/cm.c
	infiniband/core/sysfs.c
	infiniband/core/user_mad.c
	infiniband/hw/mlx4/sysfs.c
	infiniband/hw/qib/qib_sysfs.c
	infiniband/hw/usnic/usnic_ib_sysfs.c
	iommu/iommu.c
	parisc/pdc_stable.c
	pci/slot.c
	power/ab8500_fg.c
	power/abx500_chargalg.c
	scsi/iscsi_boot_sysfs.c
	uio/uio.c
	video/fbdev/omap2/dss/manager-sysfs.c
	video/fbdev/omap2/dss/overlay-sysfs.c

I'll look into them tomorrow...

But that's still a very small minority compared to the tens of thousands
of drivers we have, so I still say it's wrong to have a driver deal with
raw kobjects.

thanks,

greg k-h

Re: kobject_init and the zeroed-out-memory requirement

[Guenter Roeck]

On 10/05/2014 08:25 PM, Greg KH wrote:
> On Sun, Oct 05, 2014 at 07:09:50PM -0700, Guenter Roeck wrote:
>> On Sun, Oct 05, 2014 at 04:24:57PM -0700, Greg KH wrote:
>>> On Sun, Oct 05, 2014 at 06:13:05PM -0400, Jason Noakes wrote:
>>>>> No driver should be working with "raw" kobjects.
>>>>
>>>> I don't agree, but it's irrelevant.
>>>
>>> Not at all.  I'd wager that if a driver is messing around with a "raw"
>>> kobject, it is doing something seriously wrong.  Of course there are
>>> exceptions, but those are very rare, and exceptions.  A driver should be
>>> using the driver core, and the functions and objects provided there, and
>>> provided by the bus it lives on.
>>>
>>> So, have a pointer to some driver code that is calling
>>> kobject_initialize()?  I'd love to see it.
>>>
>>
>> Lots to choose from. In drivers/:
>>
>> $ git grep kobject_init | wc
>>       65     289    4880
>> $ git grep kobject_init | grep -v base | wc
>>       62     277    4694
>> $ git grep kobject_init_and_add | grep -v base | wc
>>       47     232    3698
>> $ git grep kobject_add | grep -v base | wc
>>       20     111    1486
>
> Well, you hit firmware layer, and block devices, which have to deal with
> kobjects directly at times.  But these files look "suspicious":
> 	edac/edac_device_sysfs.c
> 	edac/edac_pci_sysfs.c
> 	gpu/drm/ttm/ttm_bo.c
> 	gpu/drm/ttm/ttm_memory.c
> 	gpu/drm/ttm/ttm_page_alloc.c
> 	gpu/drm/ttm/ttm_page_alloc_dma.c
> 	infiniband/core/cm.c
> 	infiniband/core/sysfs.c
> 	infiniband/core/user_mad.c
> 	infiniband/hw/mlx4/sysfs.c
> 	infiniband/hw/qib/qib_sysfs.c
> 	infiniband/hw/usnic/usnic_ib_sysfs.c
> 	iommu/iommu.c
> 	parisc/pdc_stable.c
> 	pci/slot.c
> 	power/ab8500_fg.c
> 	power/abx500_chargalg.c
> 	scsi/iscsi_boot_sysfs.c
> 	uio/uio.c
> 	video/fbdev/omap2/dss/manager-sysfs.c
> 	video/fbdev/omap2/dss/overlay-sysfs.c
>
> I'll look into them tomorrow...
>
> But that's still a very small minority compared to the tens of thousands
> of drivers we have, so I still say it's wrong to have a driver deal with
> raw kobjects.
>

I agree. Just wanted to point out that there _are_ drivers out there
using it. I had actually looked into the power and video files,
and they looked suspicious to me as well.

Guenter