Re: [PATCH 1/1] linux/container_of.h: Warn about loss of constness

[Sakari Ailus]

Hi Greg,

On Mon, Oct 24, 2022 at 11:48:32AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 24, 2022 at 09:11:53AM +0000, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > Thanks for the comments.
> > 
> > On Mon, Oct 24, 2022 at 10:45:25AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 24, 2022 at 10:43:52AM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Oct 24, 2022 at 11:26:10AM +0300, Sakari Ailus wrote:
> > > > > container_of() casts the original type to another which leads to the loss
> > > > > of the const qualifier if it is not specified in the caller-provided type.
> > > > > This easily leads to container_of() returning a non-const pointer to a
> > > > > const struct which the C compiler does not warn about.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > >  include/linux/container_of.h | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> > > > > index 2f4944b791b81..c7c21d0f41a87 100644
> > > > > --- a/include/linux/container_of.h
> > > > > +++ b/include/linux/container_of.h
> > > > > @@ -13,6 +13,10 @@
> > > > >   * @type:	the type of the container struct this is embedded in.
> > > > >   * @member:	the name of the member within the struct.
> > > > >   *
> > > > > + * WARNING: as container_of() casts the given struct to another, also the
> > > > 
> > > > No need for "also" here (sorry for the grammar nit.)
> > > > 
> > > > > + * possible const qualifier of @ptr is lost unless it is also specified in
> > > > > + * @type. This is not a problem if the containing object is not const. Use with
> > > > > + * care.
> > > > 
> > > > I do not think these last two sentences you added here are needed
> > > > either.
> > > > 
> > > > 
> > > > >   */
> > > > >  #define container_of(ptr, type, member) ({				\
> > > > >  	void *__mptr = (void *)(ptr);					\
> > > > > @@ -27,6 +31,11 @@
> > > > >   * @type:	the type of the container struct this is embedded in.
> > > > >   * @member:	the name of the member within the struct.
> > > > >   *
> > > > > + * WARNING: as container_of() casts the given struct to another, also the
> > > 
> > > Wrong function name here.
> > 
> > I'll address this and the other two issues above in v2.
> > 
> > > 
> > > > > + * possible const qualifier of @ptr is lost unless it is also specified in
> > > > > + * @type. This is not a problem if the containing object is not const. Use with
> > > > > + * care.
> > > > 
> > > > Same comments here.
> > > 
> > > Wait, no one uses this macro, so why not just remove it entirely?
> > 
> > Good question. It appears to be a (relatively) common pattern to look up
> > something and the return its containing object if the lookup was
> > successful. Doing a quick
> > 
> > 	$ git grep 'container_of.*:' drivers include
> 
> And odds are, they all are wrong.
> 
> Any function that has a pointer sent to it that it wants to then cast
> out to the outer size of the structure has to implicitly know that this
> is a valid pointer.  There's no way to check so you have to trust the
> fact that the caller sent you the right thing.
> 
> Trying to check is almost always someone trying to be "over eager" in
> testing things that can never happen.  Just like all of the checks for
> the result of a container_of() call, that's always wrong as well.

I don't see how it would be more wrong than checking for NULL (or an error)
in other macros. The caller won't have to check for those separately and
this tends to avoid accidental NULL pointer dereferences.

But given that the macro was unused after four or so years suggests that we
can probably do fine without it, too.

> > reveals more than 20 instances of the pattern. There are probably more
> > those that use if for testing for NULL. I guess people don't know about
> > this macro, apart from the developers of the staging driver it was added
> > for (commit 05e6557b8ed833546ee2b66ce6b58fecf09f439e).
> 
> Ah, lustre is long-gone, so I'll just add a patch to my tree to remove
> this macro.

Ok. I'll send v2 with this in mind.

-- 
Kind regards,

Sakari Ailus