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

  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@linux.intel.com>
---
 include/linux/container_of.h | 9 +++++++++
 1 file changed, 9 insertions(+)
  

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@linux.intel.com>
> ---
>  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
> + * 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.

thanks,

greg k-h
  

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@linux.intel.com>
> > ---
> >  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.

> > + * 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?

thanks,

greg k-h
  

From: Greg Kroah-Hartman
> Sent: 24 October 2022 09:44
...
> > + * 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.

It is all TL;DR :-)

Even just:

NOTE: any const qualifier of @ptr is lost.

Is probably more than enough.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

+ Kees

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.

...

> > >   * @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.
> 
> > > + * 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?

Kees, do you know why and what for we have container_of_safe()?
  

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@linux.intel.com>
> > > ---
> > >  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

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).
  

On Mon, Oct 24, 2022 at 09:11:53AM +0000, Sakari Ailus wrote:
> 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:

...

> > 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
> 
> 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).

Maybe we can provide an example to keep this macro in the kernel, meaning
convert one of the drivers / subsystem to actually use it?
  

From: Andy Shevchenko
> Sent: 24 October 2022 10:23
...
> > > 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
> >
> > 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).
> 
> Maybe we can provide an example to keep this macro in the kernel, meaning
> convert one of the drivers / subsystem to actually use it?

Adding _safe() to a function name doesn't actually tell you anything.
You still need to look up what it is 'safe' against.

In this case the full code pattern is actually much clearer.

It is also quite likely that it is followed by an:
	if (!ptr)
		return xxx;
You that can/should really be put before the container_of() call.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Mon, Oct 24, 2022 at 09:34:42AM +0000, David Laight wrote:
> From: Andy Shevchenko
> > Sent: 24 October 2022 10:23

...

> > > > 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
> > >
> > > 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).
> > 
> > Maybe we can provide an example to keep this macro in the kernel, meaning
> > convert one of the drivers / subsystem to actually use it?
> 
> Adding _safe() to a function name doesn't actually tell you anything.
> You still need to look up what it is 'safe' against.
> 
> In this case the full code pattern is actually much clearer.
> 
> It is also quite likely that it is followed by an:
> 	if (!ptr)
> 		return xxx;
> You that can/should really be put before the container_of() call.

return statements in macros are no go. Or you meant something else?
  

From: 'Andy Shevchenko'
> Sent: 24 October 2022 10:37
> ...
> 
> > > > > 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
> > > >
> > > > 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).
> > >
> > > Maybe we can provide an example to keep this macro in the kernel, meaning
> > > convert one of the drivers / subsystem to actually use it?
> >
> > Adding _safe() to a function name doesn't actually tell you anything.
> > You still need to look up what it is 'safe' against.
> >
> > In this case the full code pattern is actually much clearer.
> >
> > It is also quite likely that it is followed by an:
> > 	if (!ptr)
> > 		return xxx;
> > You that can/should really be put before the container_of() call.
> 
> return statements in macros are no go. Or you meant something else?

I meant in the function itself.

It might be interesting to check how many of the function
can actually have a NULL pointer?
Especially in staging code might be being 'defensive'.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

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@linux.intel.com>
> > > > ---
> > > >  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.
thanks,

> 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.

thanks,

greg k-h
  

On Mon, Oct 24, 2022 at 09:46:40AM +0000, David Laight wrote:
> From: 'Andy Shevchenko'
> > Sent: 24 October 2022 10:37
> > ...
> > 
> > > > > > 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
> > > > >
> > > > > 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).
> > > >
> > > > Maybe we can provide an example to keep this macro in the kernel, meaning
> > > > convert one of the drivers / subsystem to actually use it?
> > >
> > > Adding _safe() to a function name doesn't actually tell you anything.
> > > You still need to look up what it is 'safe' against.
> > >
> > > In this case the full code pattern is actually much clearer.
> > >
> > > It is also quite likely that it is followed by an:
> > > 	if (!ptr)
> > > 		return xxx;
> > > You that can/should really be put before the container_of() call.
> > 
> > return statements in macros are no go. Or you meant something else?
> 
> I meant in the function itself.
> 
> It might be interesting to check how many of the function
> can actually have a NULL pointer?
> Especially in staging code might be being 'defensive'.

This is a pointless discussion, this macro will now be deleted, sorry.

greg k-h
  

From: Greg Kroah-Hartman
> Sent: 24 October 2022 11:02
> 
> On Mon, Oct 24, 2022 at 09:46:40AM +0000, David Laight wrote:
> > From: 'Andy Shevchenko'
> > > Sent: 24 October 2022 10:37
> > > ...
> > >
> > > > > > > Wait, no one uses this macro, so why not just remove it entirely?
...
> > It might be interesting to check how many of the function
> > can actually have a NULL pointer?
> > Especially in staging code might be being 'defensive'.
> 
> This is a pointless discussion, this macro will now be deleted, sorry.

I think we actually agree :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

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@linux.intel.com>
> > > > > ---
> > > > >  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.
  

Hi David,

On Mon, Oct 24, 2022 at 08:59:29AM +0000, David Laight wrote:
> From: Greg Kroah-Hartman
> > Sent: 24 October 2022 09:44
> ...
> > > + * 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.
> 
> It is all TL;DR :-)
> 
> Even just:
> 
> NOTE: any const qualifier of @ptr is lost.
> 
> Is probably more than enough.

Fine for me, but I'd prefer to keep the WARNING, making this:

	WARNING: any const qualifier of @ptr is lost.
  

On Mon, Oct 24, 2022 at 12:00:16PM +0300, Andy Shevchenko wrote:
> + Kees
> 
> 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.
> 
> ...
> 
> > > >   * @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.
> > 
> > > > + * 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?
> 
> Kees, do you know why and what for we have container_of_safe()?

It looks like it was designed to handle the cases where the pointer was
ERR_OR_NULL:

       IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) : \
               ((type *)(__mptr - offsetof(type, member))); })

i.e. just pass through the NULL/ERR instead of attempting the cast,
which would fail spectacularly. :)

It seems like this version should actually be used everywhere instead of
nowhere... (i.e. just drop container_of() and rename container_of_safe()
to container_of())
  

On Mon, Oct 24, 2022 at 7:39 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 24, 2022 at 12:00:16PM +0300, Andy Shevchenko wrote:
> > + Kees
> >
> > 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.
> >
> > ...
> >
> > > > >   * @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.
> > >
> > > > > + * 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?
> >
> > Kees, do you know why and what for we have container_of_safe()?
>
> It looks like it was designed to handle the cases where the pointer was
> ERR_OR_NULL:
>
>        IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) : \
>                ((type *)(__mptr - offsetof(type, member))); })
>
> i.e. just pass through the NULL/ERR instead of attempting the cast,
> which would fail spectacularly. :)
>
> It seems like this version should actually be used everywhere instead of
> nowhere... (i.e. just drop container_of() and rename container_of_safe()
> to container_of())

As a rule, though, users of container_of() don't check the pointer
returned by it against NULL, so I'm not sure how much of an
improvement that would be.

If NULL is passed to container_of(), there will be a spectacular
failure, sooner or later ...
  

From: Rafael J. Wysocki
> Sent: 24 October 2022 18:51
...
> > It looks like it was designed to handle the cases where the pointer was
> > ERR_OR_NULL:
> >
> >        IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) : \
> >                ((type *)(__mptr - offsetof(type, member))); })
> >
> > i.e. just pass through the NULL/ERR instead of attempting the cast,
> > which would fail spectacularly. :)
> >
> > It seems like this version should actually be used everywhere instead of
> > nowhere... (i.e. just drop container_of() and rename container_of_safe()
> > to container_of())
> 
> As a rule, though, users of container_of() don't check the pointer
> returned by it against NULL, so I'm not sure how much of an
> improvement that would be.
> 
> If NULL is passed to container_of(), there will be a spectacular
> failure, sooner or later ...

Certainly there isn't much difference between dereferencing
a -Exxxx value and -Exxxx - offsetof().
Both are in the same page - hopefully not mapped?

Missing ERR/NULL checks are a problem but adding one inside
container_of() doesn't really help.

You might as well add an explicit test before using container_of()
rather than adding one inside it AND requiring a test afterwards.
I don't think the compiler can assume the subtraction doesn't
generate NULL - so must check twice.

I've not even sure how many of the functions that can check can
ever actually be passed an invalid pointer.
Normally the caller bails out and returns the error before
passing it on.
The kernel really doesn't check every function parameter for
validity - it has to assume the caller is doing something sensible.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Mon, Oct 24, 2022 at 07:51:11PM +0200, Rafael J. Wysocki wrote:
> On Mon, Oct 24, 2022 at 7:39 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Oct 24, 2022 at 12:00:16PM +0300, Andy Shevchenko wrote:
> > > + Kees
> > >
> > > 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.
> > >
> > > ...
> > >
> > > > > >   * @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.
> > > >
> > > > > > + * 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?
> > >
> > > Kees, do you know why and what for we have container_of_safe()?
> >
> > It looks like it was designed to handle the cases where the pointer was
> > ERR_OR_NULL:
> >
> >        IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) : \
> >                ((type *)(__mptr - offsetof(type, member))); })
> >
> > i.e. just pass through the NULL/ERR instead of attempting the cast,
> > which would fail spectacularly. :)
> >
> > It seems like this version should actually be used everywhere instead of
> > nowhere... (i.e. just drop container_of() and rename container_of_safe()
> > to container_of())
> 
> As a rule, though, users of container_of() don't check the pointer
> returned by it against NULL, so I'm not sure how much of an
> improvement that would be.

Nor should they.  This is just tiny pointer math, that always assumes a
valid pointer is passed in.  It should never be used in any code path
where a valid pointer is NOT passed into it.

thanks,

greg k-h