[PATCH 1/1] container_of: Document container_of_const() is preferred

[PATCH 1/1] container_of: Document container_of_const() is preferred

[Sakari Ailus]

There is a warning in kerneldoc documentation of container_of() that
constness of @ptr is lost. While this is a suggestion container_of_const()
should be used instead, the vast majority of new code still uses
container_of():

$ git diff v6.8 v6.9|grep container_of\(|wc -l
788
$ git diff v6.8 v6.9|grep container_of_const|wc -l
11

Make an explicit recommendation to use container_of_const(), unless @ptr
is const but its container isn't.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/linux/container_of.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/container_of.h b/include/linux/container_of.h
index 713890c867be..7563015ff165 100644
--- a/include/linux/container_of.h
+++ b/include/linux/container_of.h
@@ -13,7 +13,9 @@
  * @type:	the type of the container struct this is embedded in.
  * @member:	the name of the member within the struct.
  *
- * WARNING: any const qualifier of @ptr is lost.
+ * WARNING: any const qualifier of @ptr is lost. container_of() should only be
+ * used in cases where @ptr is const and its container is not and you know what
+ * you're doing. Otherwise always use container_of_const().
  */
 #define container_of(ptr, type, member) ({				\
 	void *__mptr = (void *)(ptr);					\
-- 
2.39.2

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Greg Kroah-Hartman]

On Mon, Jun 17, 2024 at 01:08:25PM +0300, Sakari Ailus wrote:
> There is a warning in kerneldoc documentation of container_of() that
> constness of @ptr is lost. While this is a suggestion container_of_const()
> should be used instead, the vast majority of new code still uses
> container_of():
> 
> $ git diff v6.8 v6.9|grep container_of\(|wc -l
> 788
> $ git diff v6.8 v6.9|grep container_of_const|wc -l
> 11

That is because container_of_const is new, and you don't normally go
back and change things unless you have to.  Which is what I am starting
to do for some cases now in the driver core interactions, but generally
it's rare to need this.

Also note that container_of_const does not work in an inline function,
which is another reason people might not want to use it.

> Make an explicit recommendation to use container_of_const(), unless @ptr
> is const but its container isn't.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/linux/container_of.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> index 713890c867be..7563015ff165 100644
> --- a/include/linux/container_of.h
> +++ b/include/linux/container_of.h
> @@ -13,7 +13,9 @@
>   * @type:	the type of the container struct this is embedded in.
>   * @member:	the name of the member within the struct.
>   *
> - * WARNING: any const qualifier of @ptr is lost.
> + * WARNING: any const qualifier of @ptr is lost. container_of() should only be
> + * used in cases where @ptr is const and its container is not and you know what
> + * you're doing. Otherwise always use container_of_const().

I know of no cases where a @ptr would be const yet the container would
not be, do you?  So why say that here?  That implies that it is a valid
thing to actually do.

I don't understand the goal here, do you want to just not have new
usages use container_of() at all?  Or are you trying to warn people of a
common problem that they make?  Having a const @ptr is not normal in the
kernel, so this should be ok.  If not, send patches to fix up those
users please.

thanks,

greg k-h

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Sakari Ailus]

Hi Greg,

On Mon, Jun 17, 2024 at 12:44:55PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jun 17, 2024 at 01:08:25PM +0300, Sakari Ailus wrote:
> > There is a warning in kerneldoc documentation of container_of() that
> > constness of @ptr is lost. While this is a suggestion container_of_const()
> > should be used instead, the vast majority of new code still uses
> > container_of():
> > 
> > $ git diff v6.8 v6.9|grep container_of\(|wc -l
> > 788
> > $ git diff v6.8 v6.9|grep container_of_const|wc -l
> > 11
> 
> That is because container_of_const is new, and you don't normally go
> back and change things unless you have to.  Which is what I am starting
> to do for some cases now in the driver core interactions, but generally
> it's rare to need this.

container_of_const() does provide a useful a useful sanity check and I
think we should encourage people to use it. I'm happy to see many macros
under include/ use container_of_const() already, but there seem to be more
than 1000 cases where the constness qualifier of a pointer is just
discarded just in the scope that got compiled with my current .config (not
allyesconfig). While the vast majority are probably benign, I wouldn't be
certain there aren't cases where the container of a const pointer ends up
being modified.

> 
> Also note that container_of_const does not work in an inline function,
> which is another reason people might not want to use it.

Does not work or is less useful (compared to a macro)? _Generic() would
need to be used if you'd like to have const and non-const variants of an
inline function but I guess in most cases macros are just fine.

> 
> > Make an explicit recommendation to use container_of_const(), unless @ptr
> > is const but its container isn't.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/linux/container_of.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> > index 713890c867be..7563015ff165 100644
> > --- a/include/linux/container_of.h
> > +++ b/include/linux/container_of.h
> > @@ -13,7 +13,9 @@
> >   * @type:	the type of the container struct this is embedded in.
> >   * @member:	the name of the member within the struct.
> >   *
> > - * WARNING: any const qualifier of @ptr is lost.
> > + * WARNING: any const qualifier of @ptr is lost. container_of() should only be
> > + * used in cases where @ptr is const and its container is not and you know what
> > + * you're doing. Otherwise always use container_of_const().
> 
> I know of no cases where a @ptr would be const yet the container would
> not be, do you?  So why say that here?  That implies that it is a valid
> thing to actually do.
> 
> I don't understand the goal here, do you want to just not have new
> usages use container_of() at all?  Or are you trying to warn people of a
> common problem that they make?  Having a const @ptr is not normal in the
> kernel, so this should be ok.  If not, send patches to fix up those
> users please.

My immediate goal is to encourage people to use container_of_const() for
the added sanity check and stop adding technical debt (code that ignores
const qualifier). Currently people also do think they should be using
container_of() instead of container_of_const() because the pointer they
have is not const (at the time of writing the code at least).

Eventually (or hopefully?) adding that sanity check for container_of() may
be possible so we'd again have just one macro for the job.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Greg Kroah-Hartman]

On Tue, Jun 18, 2024 at 09:09:03AM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Mon, Jun 17, 2024 at 12:44:55PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jun 17, 2024 at 01:08:25PM +0300, Sakari Ailus wrote:
> > > There is a warning in kerneldoc documentation of container_of() that
> > > constness of @ptr is lost. While this is a suggestion container_of_const()
> > > should be used instead, the vast majority of new code still uses
> > > container_of():
> > > 
> > > $ git diff v6.8 v6.9|grep container_of\(|wc -l
> > > 788
> > > $ git diff v6.8 v6.9|grep container_of_const|wc -l
> > > 11
> > 
> > That is because container_of_const is new, and you don't normally go
> > back and change things unless you have to.  Which is what I am starting
> > to do for some cases now in the driver core interactions, but generally
> > it's rare to need this.
> 
> container_of_const() does provide a useful a useful sanity check and I
> think we should encourage people to use it. I'm happy to see many macros
> under include/ use container_of_const() already, but there seem to be more
> than 1000 cases where the constness qualifier of a pointer is just
> discarded just in the scope that got compiled with my current .config (not
> allyesconfig). While the vast majority are probably benign, I wouldn't be
> certain there aren't cases where the container of a const pointer ends up
> being modified.
> 
> > 
> > Also note that container_of_const does not work in an inline function,
> > which is another reason people might not want to use it.
> 
> Does not work or is less useful (compared to a macro)? _Generic() would
> need to be used if you'd like to have const and non-const variants of an
> inline function but I guess in most cases macros are just fine.

I could not figure out a way to make this an inline function at all.
Try it yourself and see, maybe I was wrong.

> > > Make an explicit recommendation to use container_of_const(), unless @ptr
> > > is const but its container isn't.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  include/linux/container_of.h | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> > > index 713890c867be..7563015ff165 100644
> > > --- a/include/linux/container_of.h
> > > +++ b/include/linux/container_of.h
> > > @@ -13,7 +13,9 @@
> > >   * @type:	the type of the container struct this is embedded in.
> > >   * @member:	the name of the member within the struct.
> > >   *
> > > - * WARNING: any const qualifier of @ptr is lost.
> > > + * WARNING: any const qualifier of @ptr is lost. container_of() should only be
> > > + * used in cases where @ptr is const and its container is not and you know what
> > > + * you're doing. Otherwise always use container_of_const().
> > 
> > I know of no cases where a @ptr would be const yet the container would
> > not be, do you?  So why say that here?  That implies that it is a valid
> > thing to actually do.
> > 
> > I don't understand the goal here, do you want to just not have new
> > usages use container_of() at all?  Or are you trying to warn people of a
> > common problem that they make?  Having a const @ptr is not normal in the
> > kernel, so this should be ok.  If not, send patches to fix up those
> > users please.
> 
> My immediate goal is to encourage people to use container_of_const() for
> the added sanity check and stop adding technical debt (code that ignores
> const qualifier). Currently people also do think they should be using
> container_of() instead of container_of_const() because the pointer they
> have is not const (at the time of writing the code at least).

That's fine, so for new things, use container_of_const(), but generally
the need for a const is quite rare, outside of the driver core
interactions.

> Eventually (or hopefully?) adding that sanity check for container_of() may
> be possible so we'd again have just one macro for the job.

That would be nice, try doing that and see what blows up.

thanks,

greg k-h

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Sakari Ailus]

Hi Greg,

On Tue, Jun 18, 2024 at 12:01:30PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 18, 2024 at 09:09:03AM +0000, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Mon, Jun 17, 2024 at 12:44:55PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Jun 17, 2024 at 01:08:25PM +0300, Sakari Ailus wrote:
> > > > There is a warning in kerneldoc documentation of container_of() that
> > > > constness of @ptr is lost. While this is a suggestion container_of_const()
> > > > should be used instead, the vast majority of new code still uses
> > > > container_of():
> > > > 
> > > > $ git diff v6.8 v6.9|grep container_of\(|wc -l
> > > > 788
> > > > $ git diff v6.8 v6.9|grep container_of_const|wc -l
> > > > 11
> > > 
> > > That is because container_of_const is new, and you don't normally go
> > > back and change things unless you have to.  Which is what I am starting
> > > to do for some cases now in the driver core interactions, but generally
> > > it's rare to need this.
> > 
> > container_of_const() does provide a useful a useful sanity check and I
> > think we should encourage people to use it. I'm happy to see many macros
> > under include/ use container_of_const() already, but there seem to be more
> > than 1000 cases where the constness qualifier of a pointer is just
> > discarded just in the scope that got compiled with my current .config (not
> > allyesconfig). While the vast majority are probably benign, I wouldn't be
> > certain there aren't cases where the container of a const pointer ends up
> > being modified.
> > 
> > > 
> > > Also note that container_of_const does not work in an inline function,
> > > which is another reason people might not want to use it.
> > 
> > Does not work or is less useful (compared to a macro)? _Generic() would
> > need to be used if you'd like to have const and non-const variants of an
> > inline function but I guess in most cases macros are just fine.
> 
> I could not figure out a way to make this an inline function at all.
> Try it yourself and see, maybe I was wrong.

I didn't have any issues (apart from me misspelling function names ;)) with
GCC 12, neither in using container_of_const() in a static inline function
nor in using a static inline function as a _Generic() expression.

If other compilers have issues we can update the documentation I guess? Or
only make the check on compilers that properly support it? Or in the best
case, fix those compilers. This does tend to take a long time though.

> 
> > > > Make an explicit recommendation to use container_of_const(), unless @ptr
> > > > is const but its container isn't.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  include/linux/container_of.h | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> > > > index 713890c867be..7563015ff165 100644
> > > > --- a/include/linux/container_of.h
> > > > +++ b/include/linux/container_of.h
> > > > @@ -13,7 +13,9 @@
> > > >   * @type:	the type of the container struct this is embedded in.
> > > >   * @member:	the name of the member within the struct.
> > > >   *
> > > > - * WARNING: any const qualifier of @ptr is lost.
> > > > + * WARNING: any const qualifier of @ptr is lost. container_of() should only be
> > > > + * used in cases where @ptr is const and its container is not and you know what
> > > > + * you're doing. Otherwise always use container_of_const().
> > > 
> > > I know of no cases where a @ptr would be const yet the container would
> > > not be, do you?  So why say that here?  That implies that it is a valid
> > > thing to actually do.
> > > 
> > > I don't understand the goal here, do you want to just not have new
> > > usages use container_of() at all?  Or are you trying to warn people of a
> > > common problem that they make?  Having a const @ptr is not normal in the
> > > kernel, so this should be ok.  If not, send patches to fix up those
> > > users please.
> > 
> > My immediate goal is to encourage people to use container_of_const() for
> > the added sanity check and stop adding technical debt (code that ignores
> > const qualifier). Currently people also do think they should be using
> > container_of() instead of container_of_const() because the pointer they
> > have is not const (at the time of writing the code at least).
> 
> That's fine, so for new things, use container_of_const(), but generally
> the need for a const is quite rare, outside of the driver core
> interactions.

Right, but I'm also looking to avoid drivers doing this inadvertly. Those
instances are just as much a blocker for turning container_of() const-aware
as anything else.

> 
> > Eventually (or hopefully?) adding that sanity check for container_of() may
> > be possible so we'd again have just one macro for the job.
> 
> That would be nice, try doing that and see what blows up.

Currently a lot of things but it can be done gradually, one instance at a
time.

How changing the container_of() documentation to this:

 * WARNING: any const qualifier of @ptr is lost. Use container_of_const()
 * instead.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Greg Kroah-Hartman]

On Tue, Jun 18, 2024 at 11:52:08AM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Tue, Jun 18, 2024 at 12:01:30PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 18, 2024 at 09:09:03AM +0000, Sakari Ailus wrote:
> > > Hi Greg,
> > > 
> > > On Mon, Jun 17, 2024 at 12:44:55PM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Jun 17, 2024 at 01:08:25PM +0300, Sakari Ailus wrote:
> > > > > There is a warning in kerneldoc documentation of container_of() that
> > > > > constness of @ptr is lost. While this is a suggestion container_of_const()
> > > > > should be used instead, the vast majority of new code still uses
> > > > > container_of():
> > > > > 
> > > > > $ git diff v6.8 v6.9|grep container_of\(|wc -l
> > > > > 788
> > > > > $ git diff v6.8 v6.9|grep container_of_const|wc -l
> > > > > 11
> > > > 
> > > > That is because container_of_const is new, and you don't normally go
> > > > back and change things unless you have to.  Which is what I am starting
> > > > to do for some cases now in the driver core interactions, but generally
> > > > it's rare to need this.
> > > 
> > > container_of_const() does provide a useful a useful sanity check and I
> > > think we should encourage people to use it. I'm happy to see many macros
> > > under include/ use container_of_const() already, but there seem to be more
> > > than 1000 cases where the constness qualifier of a pointer is just
> > > discarded just in the scope that got compiled with my current .config (not
> > > allyesconfig). While the vast majority are probably benign, I wouldn't be
> > > certain there aren't cases where the container of a const pointer ends up
> > > being modified.
> > > 
> > > > 
> > > > Also note that container_of_const does not work in an inline function,
> > > > which is another reason people might not want to use it.
> > > 
> > > Does not work or is less useful (compared to a macro)? _Generic() would
> > > need to be used if you'd like to have const and non-const variants of an
> > > inline function but I guess in most cases macros are just fine.
> > 
> > I could not figure out a way to make this an inline function at all.
> > Try it yourself and see, maybe I was wrong.
> 
> I didn't have any issues (apart from me misspelling function names ;)) with
> GCC 12, neither in using container_of_const() in a static inline function
> nor in using a static inline function as a _Generic() expression.

Really?  And how do you handle the pointer being either const or not,
and propagating that back out as the return type?  I'd like to see your
inline function please.

> If other compilers have issues we can update the documentation I guess? Or
> only make the check on compilers that properly support it? Or in the best
> case, fix those compilers. This does tend to take a long time though.

I tested this all with gcc-12 when I did the original work, so that
should be fine.

> > > > > Make an explicit recommendation to use container_of_const(), unless @ptr
> > > > > is const but its container isn't.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  include/linux/container_of.h | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> > > > > index 713890c867be..7563015ff165 100644
> > > > > --- a/include/linux/container_of.h
> > > > > +++ b/include/linux/container_of.h
> > > > > @@ -13,7 +13,9 @@
> > > > >   * @type:	the type of the container struct this is embedded in.
> > > > >   * @member:	the name of the member within the struct.
> > > > >   *
> > > > > - * WARNING: any const qualifier of @ptr is lost.
> > > > > + * WARNING: any const qualifier of @ptr is lost. container_of() should only be
> > > > > + * used in cases where @ptr is const and its container is not and you know what
> > > > > + * you're doing. Otherwise always use container_of_const().
> > > > 
> > > > I know of no cases where a @ptr would be const yet the container would
> > > > not be, do you?  So why say that here?  That implies that it is a valid
> > > > thing to actually do.
> > > > 
> > > > I don't understand the goal here, do you want to just not have new
> > > > usages use container_of() at all?  Or are you trying to warn people of a
> > > > common problem that they make?  Having a const @ptr is not normal in the
> > > > kernel, so this should be ok.  If not, send patches to fix up those
> > > > users please.
> > > 
> > > My immediate goal is to encourage people to use container_of_const() for
> > > the added sanity check and stop adding technical debt (code that ignores
> > > const qualifier). Currently people also do think they should be using
> > > container_of() instead of container_of_const() because the pointer they
> > > have is not const (at the time of writing the code at least).
> > 
> > That's fine, so for new things, use container_of_const(), but generally
> > the need for a const is quite rare, outside of the driver core
> > interactions.
> 
> Right, but I'm also looking to avoid drivers doing this inadvertly. Those
> instances are just as much a blocker for turning container_of() const-aware
> as anything else.

Almost no driver should be using container_of on their own, they should
be using the bus-provided macros instead.  Or if they are, they are
doing it for their own internal structures and as those are probably not
const, then there's not an issue here.

Specific examples of where you are seeing this being added where it
shouldn't be would be good.

Also, just start sweeping the tree if you want to, and send patches to
fix up where this is done incorrectly should be gladly accepted.

> > > Eventually (or hopefully?) adding that sanity check for container_of() may
> > > be possible so we'd again have just one macro for the job.
> > 
> > That would be nice, try doing that and see what blows up.
> 
> Currently a lot of things but it can be done gradually, one instance at a
> time.
> 
> How changing the container_of() documentation to this:
> 
>  * WARNING: any const qualifier of @ptr is lost. Use container_of_const()
>  * instead.

Is that really going to change anyone's opinion of what to use here?

thanks,

greg k-h

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Matthew Wilcox]

On Tue, Jun 18, 2024 at 02:54:53PM +0200, Greg Kroah-Hartman wrote:
> > I didn't have any issues (apart from me misspelling function names ;)) with
> > GCC 12, neither in using container_of_const() in a static inline function
> > nor in using a static inline function as a _Generic() expression.
> 
> Really?  And how do you handle the pointer being either const or not,
> and propagating that back out as the return type?  I'd like to see your
> inline function please.

Here's how I did it for page_folio():

#define page_folio(p)           (_Generic((p),                          \
        const struct page *:    (const struct folio *)_compound_head(p), \
        struct page *:          (struct folio *)_compound_head(p)))

Is there something differently magic about container_of() that prevents
this trick from working?

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Greg Kroah-Hartman]

On Tue, Jun 18, 2024 at 02:56:17PM +0100, Matthew Wilcox wrote:
> On Tue, Jun 18, 2024 at 02:54:53PM +0200, Greg Kroah-Hartman wrote:
> > > I didn't have any issues (apart from me misspelling function names ;)) with
> > > GCC 12, neither in using container_of_const() in a static inline function
> > > nor in using a static inline function as a _Generic() expression.
> > 
> > Really?  And how do you handle the pointer being either const or not,
> > and propagating that back out as the return type?  I'd like to see your
> > inline function please.
> 
> Here's how I did it for page_folio():
> 
> #define page_folio(p)           (_Generic((p),                          \
>         const struct page *:    (const struct folio *)_compound_head(p), \
>         struct page *:          (struct folio *)_compound_head(p)))
> 
> Is there something differently magic about container_of() that prevents
> this trick from working?

That's a #define, not an inline function, which is what I thought we
were talking about.

thanks,

greg k-h

Re: [PATCH 1/1] container_of: Document container_of_const() is preferred

[Andy Shevchenko]

On Mon, Jun 17, 2024 at 01:08:25PM +0300, Sakari Ailus wrote:
> There is a warning in kerneldoc documentation of container_of() that
> constness of @ptr is lost. While this is a suggestion container_of_const()
> should be used instead, the vast majority of new code still uses
> container_of():

A side note...

> $ git diff v6.8 v6.9|grep container_of\(|wc -l
> 788
> $ git diff v6.8 v6.9|grep container_of_const|wc -l
> 11

This is classic "Useless use of grep".

$ git log --oneline -G 'container_of\(' v6.8..v6.9 | wc -l
296

However, if you really want the _new_ code only, you have to run to `git grep`:

$ git grep -n -w container_of v6.8 | wc
20763

$ git grep -n -w container_of v6.9 | wc
20943

180 new uses.

-- 
With Best Regards,
Andy Shevchenko