Date: Tue, 18 Jun 2024 14:54:53 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Jason Gunthorpe <jgg@...dia.com>,
	Matthew Wilcox <willy@...radead.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Hans Verkuil <hverkuil@...all.nl>
Subject: Re: [PATCH 1/1] container_of: Document container_of_const() is
 preferred

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

Powered by blists - more mailing lists