[PATCH v2][next] container_of: add container_first() macro

[PATCH v2][next] container_of: add container_first() macro

[Gustavo A. R. Silva]

This is like container_of_const() but it contains an assert to
ensure that it's using the first member in the structure.

Co-developed-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---

I will be using this in my -Wflex-array-member-not-at-end patches. :)

Changes in v2:
 - Base this on container_of_const().

v1:
 - Link: https://lore.kernel.org/linux-hardening/Zu1vekikKNR5oUoM@elsanto/

 include/linux/container_of.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/container_of.h b/include/linux/container_of.h
index 713890c867be..1a5e5f32db92 100644
--- a/include/linux/container_of.h
+++ b/include/linux/container_of.h
@@ -35,4 +35,15 @@
 		default: ((type *)container_of(ptr, type, member))	\
 	)
 
+/**
+ * container_first - cast first member of a structure out to the containing
+ *                   structure and preserve the const-ness of the pointer.
+ * @ptr:             the pointer to the member.
+ * @type:            the type of the container struct this is embedded in.
+ * @member:          the name of the member within the struct.
+ */
+#define container_first(ptr, type, member) ({                           \
+	static_assert(offsetof(type, member) == 0, "not first member"); \
+	container_of_const(ptr, type, member); })
+
 #endif	/* _LINUX_CONTAINER_OF_H */
-- 
2.43.0

Re: [PATCH v2][next] container_of: add container_first() macro

[Greg KH]

On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> This is like container_of_const() but it contains an assert to
> ensure that it's using the first member in the structure.

But why?  If you "know" it's the first member, just do a normal cast.
If you don't, then you probably shouldn't be caring about this anyway,
right?

> 
> Co-developed-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> 
> I will be using this in my -Wflex-array-member-not-at-end patches. :)

Confused, I'd like to see some users first please.

thanks,

greg k-h

Re: [PATCH v2][next] container_of: add container_first() macro

[Dan Carpenter]

On Wed, Jan 29, 2025 at 06:54:48AM +0100, Greg KH wrote:
> On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> > This is like container_of_const() but it contains an assert to
> > ensure that it's using the first member in the structure.
> 
> But why?  If you "know" it's the first member, just do a normal cast.
> If you don't, then you probably shouldn't be caring about this anyway,
> right?
> 

Heh.  I had a long coversation with someone where I tried to explain four
times that casting the first member was a thing.  In the end, they were
able to accept that it works but only "accidentally."  We merged their
patch as a cleanup.

https://lore.kernel.org/all/20250116193432.716db3a2@gandalf.local.home/

We have quite a bit of code like:

drivers/iommu/iommufd/iommufd_private.h
   243  static inline struct iommufd_ioas *iommufd_get_ioas(struct iommufd_ctx *ictx,
   244                                                      u32 id)
   245  {
   246          return container_of(iommufd_get_object(ictx, id,
   247                                                 IOMMUFD_OBJ_IOAS),
   248                              struct iommufd_ioas, obj);
   249  }

It's just a cast like you say, but it looks like pointer math.  It would
be more readable as container_of_first().

The weird thing is that when people check if (IS_ERR()) on a
container_of() then normally the code is correct, but when they check for
NULL then normally the NULL check is a harmless no-op.

regards,
dan carpenter

Re: [PATCH v2][next] container_of: add container_first() macro

[Dan Carpenter]

On Wed, Jan 29, 2025 at 10:29:24AM +0300, Dan Carpenter wrote:
> drivers/iommu/iommufd/iommufd_private.h
>    243  static inline struct iommufd_ioas *iommufd_get_ioas(struct iommufd_ctx *ictx,
>    244                                                      u32 id)
>    245  {
>    246          return container_of(iommufd_get_object(ictx, id,
>    247                                                 IOMMUFD_OBJ_IOAS),
>    248                              struct iommufd_ioas, obj);
>    249  }
> 
> It's just a cast like you say, but it looks like pointer math.  It would
> be more readable as container_of_first().

I left out the important bit...  iommufd_get_object() returns error pointers.
It doesn't make sense to pass error pointers to container_of() unless the
offset is zero.

Smatch could check that the offset is zero, but Coccinelle can't.  There are
a bunch of advantages to Coccinelle at times so this will improve the
readability and make it easier for static checkers as well.

regards,
dan carpenter

Re: [PATCH v2][next] container_of: add container_first() macro

[Greg KH]

On Wed, Jan 29, 2025 at 10:33:56AM +0300, Dan Carpenter wrote:
> On Wed, Jan 29, 2025 at 10:29:24AM +0300, Dan Carpenter wrote:
> > drivers/iommu/iommufd/iommufd_private.h
> >    243  static inline struct iommufd_ioas *iommufd_get_ioas(struct iommufd_ctx *ictx,
> >    244                                                      u32 id)
> >    245  {
> >    246          return container_of(iommufd_get_object(ictx, id,
> >    247                                                 IOMMUFD_OBJ_IOAS),
> >    248                              struct iommufd_ioas, obj);
> >    249  }
> > 
> > It's just a cast like you say, but it looks like pointer math.  It would
> > be more readable as container_of_first().
> 
> I left out the important bit...  iommufd_get_object() returns error pointers.
> It doesn't make sense to pass error pointers to container_of() unless the
> offset is zero.

Ick, agreed.  you should never be checking the return value of
container_of().  Shouldn't we somehow check for that?  Is there the
inverse of __must_check where we "just" want to use the value but never
compare it to anything?

> Smatch could check that the offset is zero, but Coccinelle can't.  There are
> a bunch of advantages to Coccinelle at times so this will improve the
> readability and make it easier for static checkers as well.

But I don't see how container_first() will do anything to help the above
type of code.  Gustavo, have any examples of what you need this for?

thanks,

greg k-h

Re: [PATCH v2][next] container_of: add container_first() macro

[Gustavo A. R. Silva]

On 29/01/25 16:24, Greg KH wrote:
> On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
>> This is like container_of_const() but it contains an assert to
>> ensure that it's using the first member in the structure.
> 
> But why?  If you "know" it's the first member, just do a normal cast.
> If you don't, then you probably shouldn't be caring about this anyway,
> right?

This is more about the cases where the member _must_ be first in the
structure. See below for an example related to -Wflex-array-member-not-at-end

> 
>>
>> Co-developed-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>
>> I will be using this in my -Wflex-array-member-not-at-end patches. :)
> 
> Confused, I'd like to see some users first please.

When addressing the -Wflex-array-member-not-at-end warnings, the common
scenario is when we have to separate the flexible-array member from the
rest of the members in the flexible structure, as shown below [1]:

struct bplus_header
{
         struct_group_tagged(bplus_header_fixed, __hdr,
                 u8 flags;               /* bit 0 - high bit of first free entry offset
                                            bit 5 - we're pointed to by an fnode,
                                            the data btree or some ea or the
                                            main ea bootage pointer ea_secno
                                            bit 6 - suggest binary search (unused)
                                            bit 7 - 1 -> (internal) tree of anodes
                                            0 -> (leaf) list of extents */
                 u8 fill[3];
                 u8 n_free_nodes;        /* free nodes in following array */
                 u8 n_used_nodes;        /* used nodes in following array */
                 __le16 first_free;      /* offset from start of header to
                                            first free node in array */
         );
         union {
                 /* (internal) 2-word entries giving subtree pointers */
                 DECLARE_FLEX_ARRAY(struct bplus_internal_node, internal);
                 /* (external) 3-word entries giving sector runs */
                 DECLARE_FLEX_ARRAY(struct bplus_leaf_node, external);
         } u;
};

struct_group_tagged() creates a new type: `struct bplus_header_fixed`, we
then use the newly created type to change the type of the middle objects
causing the warnings, and with that the warnings are gone:

@@ -453,7 +455,7 @@ struct fnode
    __le16 flags;				/* bit 1 set -> ea_secno is an anode */
  					/* bit 8 set -> directory.  first & only extent
  					   points to dnode. */
-  struct bplus_header btree;		/* b+ tree, 8 extents or 12 subtrees */
+  struct bplus_header_fixed btree;	/* b+ tree, 8 extents or 12 subtrees */
    union {
      struct bplus_leaf_node external[8];
      struct bplus_internal_node internal[12];
@@ -495,7 +497,7 @@ struct anode
    __le32 self;				/* pointer to this anode */
    __le32 up;				/* parent anode or fnode */

-  struct bplus_header btree;		/* b+tree, 40 extents or 60 subtrees */
+  struct bplus_header_fixed btree;	/* b+tree, 40 extents or 60 subtrees */
    union {
      struct bplus_leaf_node external[40];
      struct bplus_internal_node internal[60];

However, this newly created type, or rather the member `__hdr` (also
created when calling struct_group_tagged()) _must_ always be the first
member in the flexible struct `struct bplus_header`.

Then we need to use container_first() to retrieve a pointer to the flexible
structure [2], via which we can access the flexible-array member when
necessary:

diff --git a/fs/hpfs/anode.c b/fs/hpfs/anode.c
index c14c9a035ee0c0..a366f6ac71e436 100644
--- a/fs/hpfs/anode.c
+++ b/fs/hpfs/anode.c
@@ -27,7 +27,7 @@ secno hpfs_bplus_lookup(struct super_block *s, struct inode *inode,
  				a = le32_to_cpu(btree->u.internal[i].down);
  				brelse(bh);
  				if (!(anode = hpfs_map_anode(s, a, &bh))) return -1;
-				btree = &anode->btree;
+				btree = container_first(&anode->btree, struct bplus_header, __hdr);
  				goto go_down;
  			}
  		hpfs_error(s, "sector %08x not found in internal anode %08x", sec, a);
@@ -69,12 +69,16 @@ secno hpfs_add_sector_to_btree(struct super_block *s, secno node, int fnod, unsi
  	int n;
  	unsigned fs;
  	int c1, c2 = 0;
+	struct bplus_header *anode_btree = container_first(&anode->btree, struct bplus_header, __hdr);
+	struct bplus_header *ranode_btree = container_first(&ranode->btree, struct bplus_header, __hdr);
+	struct bplus_header *fnode_btree = container_first(&fnode->btree, struct bplus_header, __hdr);
+

So, if we use container_first() (or container_of_first(), you tell
which name you prefer), we are asserting that that `__hdr` member
is always the first member in `struct bplus_header`.

I've explained all this at Plumbers last year. In any case, let
me know if you need more clarification. :)

Thanks!
--
Gustavo

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/diff/fs/hpfs/hpfs.h?h=testing/wfamnae-next20250124&id=f66219294267a2fba220f4f3118e11c5cda63d0b
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/diff/fs/hpfs/anode.c?h=testing/wfamnae-next20250124&id=f66219294267a2fba220f4f3118e11c5cda63d0b

Re: [PATCH v2][next] container_of: add container_first() macro

[Greg KH]

On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote:
> 
> 
> On 29/01/25 16:24, Greg KH wrote:
> > On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> > > This is like container_of_const() but it contains an assert to
> > > ensure that it's using the first member in the structure.
> > 
> > But why?  If you "know" it's the first member, just do a normal cast.
> > If you don't, then you probably shouldn't be caring about this anyway,
> > right?
> 
> This is more about the cases where the member _must_ be first in the
> structure. See below for an example related to -Wflex-array-member-not-at-end

That's fine, but that's a build-time issue, you should enforce that in
the structure itself, why are you forcing people to remember to use this
macro when you want to use the field?  There's nothing preventing anyone
from using container_of() instead here, and nothing will catch that from
what I can tell.

> When addressing the -Wflex-array-member-not-at-end warnings, the common
> scenario is when we have to separate the flexible-array member from the
> rest of the members in the flexible structure, as shown below [1]:
> 
> struct bplus_header
> {
>         struct_group_tagged(bplus_header_fixed, __hdr,
>                 u8 flags;               /* bit 0 - high bit of first free entry offset
>                                            bit 5 - we're pointed to by an fnode,
>                                            the data btree or some ea or the
>                                            main ea bootage pointer ea_secno
>                                            bit 6 - suggest binary search (unused)
>                                            bit 7 - 1 -> (internal) tree of anodes
>                                            0 -> (leaf) list of extents */
>                 u8 fill[3];
>                 u8 n_free_nodes;        /* free nodes in following array */
>                 u8 n_used_nodes;        /* used nodes in following array */
>                 __le16 first_free;      /* offset from start of header to
>                                            first free node in array */
>         );
>         union {
>                 /* (internal) 2-word entries giving subtree pointers */
>                 DECLARE_FLEX_ARRAY(struct bplus_internal_node, internal);
>                 /* (external) 3-word entries giving sector runs */
>                 DECLARE_FLEX_ARRAY(struct bplus_leaf_node, external);
>         } u;
> };
> 
> struct_group_tagged() creates a new type: `struct bplus_header_fixed`, we
> then use the newly created type to change the type of the middle objects
> causing the warnings, and with that the warnings are gone:
> 
> @@ -453,7 +455,7 @@ struct fnode
>    __le16 flags;				/* bit 1 set -> ea_secno is an anode */
>  					/* bit 8 set -> directory.  first & only extent
>  					   points to dnode. */
> -  struct bplus_header btree;		/* b+ tree, 8 extents or 12 subtrees */
> +  struct bplus_header_fixed btree;	/* b+ tree, 8 extents or 12 subtrees */
>    union {
>      struct bplus_leaf_node external[8];
>      struct bplus_internal_node internal[12];
> @@ -495,7 +497,7 @@ struct anode
>    __le32 self;				/* pointer to this anode */
>    __le32 up;				/* parent anode or fnode */
> 
> -  struct bplus_header btree;		/* b+tree, 40 extents or 60 subtrees */
> +  struct bplus_header_fixed btree;	/* b+tree, 40 extents or 60 subtrees */
>    union {
>      struct bplus_leaf_node external[40];
>      struct bplus_internal_node internal[60];
> 
> However, this newly created type, or rather the member `__hdr` (also
> created when calling struct_group_tagged()) _must_ always be the first
> member in the flexible struct `struct bplus_header`.
> 
> Then we need to use container_first() to retrieve a pointer to the flexible
> structure [2], via which we can access the flexible-array member when
> necessary:

You don't _need_ to use it, you could use container_of() just fine.
Which is my point, this is always going to get stale and wrong over
time, you are trying to enforce in the .c code when someone access a
field that should have been checked in the .h definition.

> diff --git a/fs/hpfs/anode.c b/fs/hpfs/anode.c
> index c14c9a035ee0c0..a366f6ac71e436 100644
> --- a/fs/hpfs/anode.c
> +++ b/fs/hpfs/anode.c
> @@ -27,7 +27,7 @@ secno hpfs_bplus_lookup(struct super_block *s, struct inode *inode,
>  				a = le32_to_cpu(btree->u.internal[i].down);
>  				brelse(bh);
>  				if (!(anode = hpfs_map_anode(s, a, &bh))) return -1;
> -				btree = &anode->btree;
> +				btree = container_first(&anode->btree, struct bplus_header, __hdr);

Ick, so when is anyone going to "know" that container_of() couldn't be
used here instead?  What is going to enforce this?



>  				goto go_down;
>  			}
>  		hpfs_error(s, "sector %08x not found in internal anode %08x", sec, a);
> @@ -69,12 +69,16 @@ secno hpfs_add_sector_to_btree(struct super_block *s, secno node, int fnod, unsi
>  	int n;
>  	unsigned fs;
>  	int c1, c2 = 0;
> +	struct bplus_header *anode_btree = container_first(&anode->btree, struct bplus_header, __hdr);
> +	struct bplus_header *ranode_btree = container_first(&ranode->btree, struct bplus_header, __hdr);
> +	struct bplus_header *fnode_btree = container_first(&fnode->btree, struct bplus_header, __hdr);
> +
> 
> So, if we use container_first() (or container_of_first(), you tell
> which name you prefer), we are asserting that that `__hdr` member
> is always the first member in `struct bplus_header`.

Yes, you are asserting it in the c code, but a global replace of
container_first() with container_of() isn't going to do anything if that
starts to happen.  And a driver/fs author doesn't care here which one
matters, they will just use the first one that compiles and runs
properly.

In other words, nothing is going to enforce this check making it
semi-useless over time.  Can't you put something in the .h file to
"ensure" that the field is first there?  Then a normal container_of()
can just be used like everyone is used to without having to worry about
anything over time.

thanks,

greg k-h

Re: [PATCH v2][next] container_of: add container_first() macro

[Dan Carpenter]

On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote:
> On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote:
> > 
> > 
> > On 29/01/25 16:24, Greg KH wrote:
> > > On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> > > > This is like container_of_const() but it contains an assert to
> > > > ensure that it's using the first member in the structure.
> > > 
> > > But why?  If you "know" it's the first member, just do a normal cast.
> > > If you don't, then you probably shouldn't be caring about this anyway,
> > > right?
> > 
> > This is more about the cases where the member _must_ be first in the
> > structure. See below for an example related to -Wflex-array-member-not-at-end
> 
> That's fine, but that's a build-time issue, you should enforce that in
> the structure itself, why are you forcing people to remember to use this
> macro when you want to use the field?  There's nothing preventing anyone
> from using container_of() instead here, and nothing will catch that from
> what I can tell.

The new definition has a static_assert() in it so it's enforced about
build time.

+#define container_first(ptr, type, member) ({                           \
+       static_assert(offsetof(type, member) == 0, "not first member"); \
+       container_of_const(ptr, type, member); })

That was the discussion at plumbers, Gustavo just wanted to use
container_of() but I told him I was tired of code which assumes that
container_of() is just a cast.  If we're going to write code with that
assumption then lets create a different macro for it and let's make the
build break if someone changes it.

regards,
dan carpenter

Re: [PATCH v2][next] container_of: add container_first() macro

[Greg KH]

On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote:
> On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote:
> > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote:
> > > 
> > > 
> > > On 29/01/25 16:24, Greg KH wrote:
> > > > On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> > > > > This is like container_of_const() but it contains an assert to
> > > > > ensure that it's using the first member in the structure.
> > > > 
> > > > But why?  If you "know" it's the first member, just do a normal cast.
> > > > If you don't, then you probably shouldn't be caring about this anyway,
> > > > right?
> > > 
> > > This is more about the cases where the member _must_ be first in the
> > > structure. See below for an example related to -Wflex-array-member-not-at-end
> > 
> > That's fine, but that's a build-time issue, you should enforce that in
> > the structure itself, why are you forcing people to remember to use this
> > macro when you want to use the field?  There's nothing preventing anyone
> > from using container_of() instead here, and nothing will catch that from
> > what I can tell.
> 
> The new definition has a static_assert() in it so it's enforced about
> build time.

Yes, but that forces you to "know" to do that in the .c file.  How do
you know to use this, and if you remove it or change it to
container_of(), it works just fine again.

> +#define container_first(ptr, type, member) ({                           \
> +       static_assert(offsetof(type, member) == 0, "not first member"); \
> +       container_of_const(ptr, type, member); })
> 
> That was the discussion at plumbers, Gustavo just wanted to use
> container_of() but I told him I was tired of code which assumes that
> container_of() is just a cast.  If we're going to write code with that
> assumption then lets create a different macro for it and let's make the
> build break if someone changes it.

That's fine, but it should be where the variable layout is, NOT where
you dereference the pointer as at that point in time, you don't know or
care if the location is in the first location at all.

thanks,

greg k-h

Re: [PATCH v2][next] container_of: add container_first() macro

[Dan Carpenter]

On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote:
> On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote:
> > On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote:
> > > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote:
> > > > 
> > > > 
> > > > On 29/01/25 16:24, Greg KH wrote:
> > > > > On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> > > > > > This is like container_of_const() but it contains an assert to
> > > > > > ensure that it's using the first member in the structure.
> > > > > 
> > > > > But why?  If you "know" it's the first member, just do a normal cast.
> > > > > If you don't, then you probably shouldn't be caring about this anyway,
> > > > > right?
> > > > 
> > > > This is more about the cases where the member _must_ be first in the
> > > > structure. See below for an example related to -Wflex-array-member-not-at-end
> > > 
> > > That's fine, but that's a build-time issue, you should enforce that in
> > > the structure itself, why are you forcing people to remember to use this
> > > macro when you want to use the field?  There's nothing preventing anyone
> > > from using container_of() instead here, and nothing will catch that from
> > > what I can tell.
> > 
> > The new definition has a static_assert() in it so it's enforced about
> > build time.
> 
> Yes, but that forces you to "know" to do that in the .c file.  How do
> you know to use this, and if you remove it or change it to
> container_of(), it works just fine again.
> 

I guess my use case is different from Gustavo's.  For him, using
container_of() is fine.  We probably don't even need an assert because
once you see a struct_group_tagged() then you know the order is important.

For me, it's code like I mentioned which does:

	p = container_of();
	if (IS_ERR(p))
		...

And I did see you suggest that people re-write that kind of code, but no
one is going to do that.  :P  People know that container_of() is just a
cast in that case.  It works fine.  It's just a bit ugly.

regards,
dan carpenter

Re: [PATCH v2][next] container_of: add container_first() macro

[Greg KH]

On Wed, Jan 29, 2025 at 05:06:54PM +0300, Dan Carpenter wrote:
> On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote:
> > On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote:
> > > On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote:
> > > > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote:
> > > > > 
> > > > > 
> > > > > On 29/01/25 16:24, Greg KH wrote:
> > > > > > On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> > > > > > > This is like container_of_const() but it contains an assert to
> > > > > > > ensure that it's using the first member in the structure.
> > > > > > 
> > > > > > But why?  If you "know" it's the first member, just do a normal cast.
> > > > > > If you don't, then you probably shouldn't be caring about this anyway,
> > > > > > right?
> > > > > 
> > > > > This is more about the cases where the member _must_ be first in the
> > > > > structure. See below for an example related to -Wflex-array-member-not-at-end
> > > > 
> > > > That's fine, but that's a build-time issue, you should enforce that in
> > > > the structure itself, why are you forcing people to remember to use this
> > > > macro when you want to use the field?  There's nothing preventing anyone
> > > > from using container_of() instead here, and nothing will catch that from
> > > > what I can tell.
> > > 
> > > The new definition has a static_assert() in it so it's enforced about
> > > build time.
> > 
> > Yes, but that forces you to "know" to do that in the .c file.  How do
> > you know to use this, and if you remove it or change it to
> > container_of(), it works just fine again.
> > 
> 
> I guess my use case is different from Gustavo's.  For him, using
> container_of() is fine.  We probably don't even need an assert because
> once you see a struct_group_tagged() then you know the order is important.

Who knows this?  The developer?  What are they supposed to "know" here?
I sure don't :)

Having some sort of "__must_be_first" marking for a field is fine and
break the build if that doesn't happen.  Otherwise this is something
that is not going to be used properly over time.

> For me, it's code like I mentioned which does:
> 
> 	p = container_of();
> 	if (IS_ERR(p))
> 		...
> 
> And I did see you suggest that people re-write that kind of code, but no
> one is going to do that.  :P  People know that container_of() is just a
> cast in that case.  It works fine.  It's just a bit ugly.

It doesn't work if the field isn't first, so no, it shouldn't be working
fine, and that should be flagged and fixed and never allowed to come
back again.  Can't we do that with coccinelle?

thanks,

greg k-h

Re: [PATCH v2][next] container_of: add container_first() macro

[Gustavo A. R. Silva]

On 30/01/25 02:38, Greg KH wrote:
> On Wed, Jan 29, 2025 at 05:06:54PM +0300, Dan Carpenter wrote:
>> On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote:
>>> On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote:
>>>> On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote:
>>>>> On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote:
>>>>>>
>>>>>>
>>>>>> On 29/01/25 16:24, Greg KH wrote:
>>>>>>> On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
>>>>>>>> This is like container_of_const() but it contains an assert to
>>>>>>>> ensure that it's using the first member in the structure.
>>>>>>>
>>>>>>> But why?  If you "know" it's the first member, just do a normal cast.
>>>>>>> If you don't, then you probably shouldn't be caring about this anyway,
>>>>>>> right?
>>>>>>
>>>>>> This is more about the cases where the member _must_ be first in the
>>>>>> structure. See below for an example related to -Wflex-array-member-not-at-end
>>>>>
>>>>> That's fine, but that's a build-time issue, you should enforce that in
>>>>> the structure itself, why are you forcing people to remember to use this
>>>>> macro when you want to use the field?  There's nothing preventing anyone
>>>>> from using container_of() instead here, and nothing will catch that from
>>>>> what I can tell.
>>>>
>>>> The new definition has a static_assert() in it so it's enforced about
>>>> build time.
>>>
>>> Yes, but that forces you to "know" to do that in the .c file.  How do
>>> you know to use this, and if you remove it or change it to
>>> container_of(), it works just fine again.
>>>
>>
>> I guess my use case is different from Gustavo's.  For him, using
>> container_of() is fine.  We probably don't even need an assert because
>> once you see a struct_group_tagged() then you know the order is important.
> 
> Who knows this?  The developer?  What are they supposed to "know" here?
> I sure don't :)
> 
> Having some sort of "__must_be_first" marking for a field is fine and
> break the build if that doesn't happen.  Otherwise this is something
> that is not going to be used properly over time.

I'm currently dealing with this situation in the following way:

  struct libipw_hdr_3addr {
-	__le16 frame_ctl;
-	__le16 duration_id;
-	u8 addr1[ETH_ALEN];
-	u8 addr2[ETH_ALEN];
-	u8 addr3[ETH_ALEN];
-	__le16 seq_ctl;
+	/* New members MUST be added within the __struct_group() macro below. */
+	__struct_group(libipw_hdr_3addr_hdr, hdr, __packed,
+		__le16 frame_ctl;
+		__le16 duration_id;
+		u8 addr1[ETH_ALEN];
+		u8 addr2[ETH_ALEN];
+		u8 addr3[ETH_ALEN];
+		__le16 seq_ctl;
+	);
  	u8 payload[];
  } __packed;
+static_assert(offsetof(struct libipw_hdr_3addr, payload) == sizeof(struct libipw_hdr_3addr_hdr),
+	      "struct member likely outside of __struct_group()");

"We also want to ensure that when new members need to be added to the
flexible structure, they are always included within the newly created
tagged struct. For this, we use `static_assert()`. This ensures that the
memory layout for both the flexible structure and the new tagged struct
is the same after any changes." [1]

We could probably create struct_group_first() instead of container_first().

Anyways, so far so good.  For some reason I was under the impression
that you two guys wanted the container_first() macro.

OK, that's it from my side.

Have a good one!
-Gustavo

[1] https://git.kernel.org/linus/089332e703b681c8

> 
>> For me, it's code like I mentioned which does:
>>
>> 	p = container_of();
>> 	if (IS_ERR(p))
>> 		...
>>
>> And I did see you suggest that people re-write that kind of code, but no
>> one is going to do that.  :P  People know that container_of() is just a
>> cast in that case.  It works fine.  It's just a bit ugly.
> 
> It doesn't work if the field isn't first, so no, it shouldn't be working
> fine, and that should be flagged and fixed and never allowed to come
> back again.  Can't we do that with coccinelle?
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v2][next] container_of: add container_first() macro

[Greg KH]

On Thu, Jan 30, 2025 at 10:06:00AM +1030, Gustavo A. R. Silva wrote:
> 
> 
> On 30/01/25 02:38, Greg KH wrote:
> > On Wed, Jan 29, 2025 at 05:06:54PM +0300, Dan Carpenter wrote:
> > > On Wed, Jan 29, 2025 at 02:14:14PM +0100, Greg KH wrote:
> > > > On Wed, Jan 29, 2025 at 01:39:27PM +0300, Dan Carpenter wrote:
> > > > > On Wed, Jan 29, 2025 at 09:34:07AM +0100, Greg KH wrote:
> > > > > > On Wed, Jan 29, 2025 at 06:35:18PM +1030, Gustavo A. R. Silva wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 29/01/25 16:24, Greg KH wrote:
> > > > > > > > On Wed, Jan 29, 2025 at 03:56:01PM +1030, Gustavo A. R. Silva wrote:
> > > > > > > > > This is like container_of_const() but it contains an assert to
> > > > > > > > > ensure that it's using the first member in the structure.
> > > > > > > > 
> > > > > > > > But why?  If you "know" it's the first member, just do a normal cast.
> > > > > > > > If you don't, then you probably shouldn't be caring about this anyway,
> > > > > > > > right?
> > > > > > > 
> > > > > > > This is more about the cases where the member _must_ be first in the
> > > > > > > structure. See below for an example related to -Wflex-array-member-not-at-end
> > > > > > 
> > > > > > That's fine, but that's a build-time issue, you should enforce that in
> > > > > > the structure itself, why are you forcing people to remember to use this
> > > > > > macro when you want to use the field?  There's nothing preventing anyone
> > > > > > from using container_of() instead here, and nothing will catch that from
> > > > > > what I can tell.
> > > > > 
> > > > > The new definition has a static_assert() in it so it's enforced about
> > > > > build time.
> > > > 
> > > > Yes, but that forces you to "know" to do that in the .c file.  How do
> > > > you know to use this, and if you remove it or change it to
> > > > container_of(), it works just fine again.
> > > > 
> > > 
> > > I guess my use case is different from Gustavo's.  For him, using
> > > container_of() is fine.  We probably don't even need an assert because
> > > once you see a struct_group_tagged() then you know the order is important.
> > 
> > Who knows this?  The developer?  What are they supposed to "know" here?
> > I sure don't :)
> > 
> > Having some sort of "__must_be_first" marking for a field is fine and
> > break the build if that doesn't happen.  Otherwise this is something
> > that is not going to be used properly over time.
> 
> I'm currently dealing with this situation in the following way:
> 
>  struct libipw_hdr_3addr {
> -	__le16 frame_ctl;
> -	__le16 duration_id;
> -	u8 addr1[ETH_ALEN];
> -	u8 addr2[ETH_ALEN];
> -	u8 addr3[ETH_ALEN];
> -	__le16 seq_ctl;
> +	/* New members MUST be added within the __struct_group() macro below. */
> +	__struct_group(libipw_hdr_3addr_hdr, hdr, __packed,
> +		__le16 frame_ctl;
> +		__le16 duration_id;
> +		u8 addr1[ETH_ALEN];
> +		u8 addr2[ETH_ALEN];
> +		u8 addr3[ETH_ALEN];
> +		__le16 seq_ctl;
> +	);
>  	u8 payload[];
>  } __packed;
> +static_assert(offsetof(struct libipw_hdr_3addr, payload) == sizeof(struct libipw_hdr_3addr_hdr),
> +	      "struct member likely outside of __struct_group()");
> 
> "We also want to ensure that when new members need to be added to the
> flexible structure, they are always included within the newly created
> tagged struct. For this, we use `static_assert()`. This ensures that the
> memory layout for both the flexible structure and the new tagged struct
> is the same after any changes." [1]

That's nice, much better and it is right where the structure is defined
making it more likely to stay there over time.

> We could probably create struct_group_first() instead of container_first().

Like this?

	#define struct_group_first(s, n)	static_assert(offsetof(s, n) == sizeof(s), "struct member likely outside of __struct_group(), please fix!")

And then the above would be:
struct_group_fist(struct libipw_hdr_3addr, payload);

Hm, use of "payload" here feels odd but you get the idea.

> Anyways, so far so good.  For some reason I was under the impression
> that you two guys wanted the container_first() macro.

I don't :)

thanks,

greg k-h

Re: [PATCH v2][next] container_of: add container_first() macro

[Dan Carpenter]

On Thu, Jan 30, 2025 at 07:14:01AM +0100, Greg KH wrote:
> > Anyways, so far so good.  For some reason I was under the impression
> > that you two guys wanted the container_first() macro.
> 
> I don't :)

Greg, container_first() is like peeing in the shower.  Once you try it
you'll never go back.  :P  Anyway, it's not really related to the work
you're doing here so don't worry about it.

regards,
dan carpenter