Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Oct 2022 11:48:32 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Sakari Ailus <sakari.ailus@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        David Laight <David.Laight@aculab.com>
Subject: Re: [PATCH 1/1] linux/container_of.h: Warn about loss of constness
Message-ID: <Y1ZfcOxnAzIO5gKc@kroah.com>
References: <20221024082610.74990-1-sakari.ailus@linux.intel.com>
 <Y1ZQSEMLkybFCadS@kroah.com>
 <Y1ZQpcdK4sdy+5QZ@kroah.com>
 <Y1ZW2WYli7Bfioxr@paasikivi.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y1ZW2WYli7Bfioxr@paasikivi.fi.intel.com>
Precedence: bulk

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