[PATCH 1/2] bcachefs: Remove incorrect __counted_by annotation

[PATCH 1/2] bcachefs: Remove incorrect __counted_by annotation

[Alan Huang]

This actually reverts 86e92eeeb237 ("bcachefs: Annotate struct bch_xattr
with __counted_by()").

After the x_name, there is a value. According to the disscussion[1],
__counted_by assumes that the flexible array member contains exactly
the amount of elements that are specified. Now there are users came across
a false positive detection of an out of bounds write caused by
the __counted_by here[2], so revert that.

[1] https://lore.kernel.org/lkml/Zv8VDKWN1GzLRT-_@archlinux/T/#m0ce9541c5070146320efd4f928cc1ff8de69e9b2
[2] https://privatebin.net/?a0d4e97d590d71e1#9bLmp2Kb5NU6X6cZEucchDcu88HzUQwHUah8okKPReEt

Signed-off-by: Alan Huang <mmpgouride@gmail.com>
---
 fs/bcachefs/xattr_format.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/bcachefs/xattr_format.h b/fs/bcachefs/xattr_format.h
index c7916011ef34..67426e33d04e 100644
--- a/fs/bcachefs/xattr_format.h
+++ b/fs/bcachefs/xattr_format.h
@@ -13,7 +13,13 @@ struct bch_xattr {
 	__u8			x_type;
 	__u8			x_name_len;
 	__le16			x_val_len;
-	__u8			x_name[] __counted_by(x_name_len);
+	/*
+	 * x_name contains the name and value counted by
+	 * x_name_len + x_val_len. The introduction of
+	 * __counted_by(x_name_len) caused a false positive
+	 * detection of an out of bounds write.
+	 */
+	__u8			x_name[];
 } __packed __aligned(8);
 
 #endif /* _BCACHEFS_XATTR_FORMAT_H */
-- 
2.48.1

[PATCH 2/2] bcachefs: Rename x_name to x_name_and_value

[Alan Huang]

The flexible array contains name and value, the x_name is misleading.

Signed-off-by: Alan Huang <mmpgouride@gmail.com>
---
 fs/bcachefs/xattr.c        | 16 ++++++++--------
 fs/bcachefs/xattr.h        |  4 ++--
 fs/bcachefs/xattr_format.h |  4 ++--
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/bcachefs/xattr.c b/fs/bcachefs/xattr.c
index 3d324e485ee9..b8bc2fb04f15 100644
--- a/fs/bcachefs/xattr.c
+++ b/fs/bcachefs/xattr.c
@@ -38,7 +38,7 @@ static u64 xattr_hash_bkey(const struct bch_hash_info *info, struct bkey_s_c k)
 	struct bkey_s_c_xattr x = bkey_s_c_to_xattr(k);
 
 	return bch2_xattr_hash(info,
-		 &X_SEARCH(x.v->x_type, x.v->x_name, x.v->x_name_len));
+		 &X_SEARCH(x.v->x_type, x.v->x_name_and_value, x.v->x_name_len));
 }
 
 static bool xattr_cmp_key(struct bkey_s_c _l, const void *_r)
@@ -48,7 +48,7 @@ static bool xattr_cmp_key(struct bkey_s_c _l, const void *_r)
 
 	return l.v->x_type != r->type ||
 		l.v->x_name_len != r->name.len ||
-		memcmp(l.v->x_name, r->name.name, r->name.len);
+		memcmp(l.v->x_name_and_value, r->name.name, r->name.len);
 }
 
 static bool xattr_cmp_bkey(struct bkey_s_c _l, struct bkey_s_c _r)
@@ -58,7 +58,7 @@ static bool xattr_cmp_bkey(struct bkey_s_c _l, struct bkey_s_c _r)
 
 	return l.v->x_type != r.v->x_type ||
 		l.v->x_name_len != r.v->x_name_len ||
-		memcmp(l.v->x_name, r.v->x_name, r.v->x_name_len);
+		memcmp(l.v->x_name_and_value, r.v->x_name_and_value, r.v->x_name_len);
 }
 
 const struct bch_hash_desc bch2_xattr_hash_desc = {
@@ -96,7 +96,7 @@ int bch2_xattr_validate(struct bch_fs *c, struct bkey_s_c k,
 			 c, xattr_invalid_type,
 			 "invalid type (%u)", xattr.v->x_type);
 
-	bkey_fsck_err_on(memchr(xattr.v->x_name, '\0', xattr.v->x_name_len),
+	bkey_fsck_err_on(memchr(xattr.v->x_name_and_value, '\0', xattr.v->x_name_len),
 			 c, xattr_name_invalid_chars,
 			 "xattr name has invalid characters");
 fsck_err:
@@ -120,13 +120,13 @@ void bch2_xattr_to_text(struct printbuf *out, struct bch_fs *c,
 	unsigned name_len = xattr.v->x_name_len;
 	unsigned val_len  = le16_to_cpu(xattr.v->x_val_len);
 	unsigned max_name_val_bytes = bkey_val_bytes(xattr.k) -
-		offsetof(struct bch_xattr, x_name);
+		offsetof(struct bch_xattr, x_name_and_value);
 
 	val_len  = min_t(int, val_len, max_name_val_bytes - name_len);
 	name_len = min(name_len, max_name_val_bytes);
 
 	prt_printf(out, "%.*s:%.*s",
-		   name_len, xattr.v->x_name,
+		   name_len, xattr.v->x_name_and_value,
 		   val_len,  (char *) xattr_val(xattr.v));
 
 	if (xattr.v->x_type == KEY_TYPE_XATTR_INDEX_POSIX_ACL_ACCESS ||
@@ -202,7 +202,7 @@ int bch2_xattr_set(struct btree_trans *trans, subvol_inum inum,
 		xattr->v.x_type		= type;
 		xattr->v.x_name_len	= namelen;
 		xattr->v.x_val_len	= cpu_to_le16(size);
-		memcpy(xattr->v.x_name, name, namelen);
+		memcpy(xattr->v.x_name_and_value, name, namelen);
 		memcpy(xattr_val(&xattr->v), value, size);
 
 		ret = bch2_hash_set(trans, bch2_xattr_hash_desc, hash_info,
@@ -270,7 +270,7 @@ static int bch2_xattr_emit(struct dentry *dentry,
 	if (!prefix)
 		return 0;
 
-	return __bch2_xattr_emit(prefix, xattr->x_name, xattr->x_name_len, buf);
+	return __bch2_xattr_emit(prefix, xattr->x_name_and_value, xattr->x_name_len, buf);
 }
 
 static int bch2_xattr_list_bcachefs(struct bch_fs *c,
diff --git a/fs/bcachefs/xattr.h b/fs/bcachefs/xattr.h
index 132fbbd15a66..1139bf345f70 100644
--- a/fs/bcachefs/xattr.h
+++ b/fs/bcachefs/xattr.h
@@ -18,12 +18,12 @@ void bch2_xattr_to_text(struct printbuf *, struct bch_fs *, struct bkey_s_c);
 
 static inline unsigned xattr_val_u64s(unsigned name_len, unsigned val_len)
 {
-	return DIV_ROUND_UP(offsetof(struct bch_xattr, x_name) +
+	return DIV_ROUND_UP(offsetof(struct bch_xattr, x_name_and_value) +
 			    name_len + val_len, sizeof(u64));
 }
 
 #define xattr_val(_xattr)					\
-	((void *) (_xattr)->x_name + (_xattr)->x_name_len)
+	((void *) (_xattr)->x_name_and_value + (_xattr)->x_name_len)
 
 struct xattr_search_key {
 	u8		type;
diff --git a/fs/bcachefs/xattr_format.h b/fs/bcachefs/xattr_format.h
index 67426e33d04e..4121b78d9a92 100644
--- a/fs/bcachefs/xattr_format.h
+++ b/fs/bcachefs/xattr_format.h
@@ -16,10 +16,10 @@ struct bch_xattr {
 	/*
 	 * x_name contains the name and value counted by
 	 * x_name_len + x_val_len. The introduction of
-	 * __counted_by(x_name_len) caused a false positive
+	 * __counted_by(x_name_len) previously caused a false positive
 	 * detection of an out of bounds write.
 	 */
-	__u8			x_name[];
+	__u8			x_name_and_value[];
 } __packed __aligned(8);
 
 #endif /* _BCACHEFS_XATTR_FORMAT_H */
-- 
2.48.1

Re: [PATCH 2/2] bcachefs: Rename x_name to x_name_and_value

[Jan Hendrik Farr]

On 02 04:01:32, Alan Huang wrote:
> The flexible array contains name and value, the x_name is misleading.
> 
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>

Reviewed-by: Jan Hendrik Farr <kernel@jfarr.cc>

> ---
>  fs/bcachefs/xattr.c        | 16 ++++++++--------
>  fs/bcachefs/xattr.h        |  4 ++--
>  fs/bcachefs/xattr_format.h |  4 ++--
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/bcachefs/xattr.c b/fs/bcachefs/xattr.c
> index 3d324e485ee9..b8bc2fb04f15 100644
> --- a/fs/bcachefs/xattr.c
> +++ b/fs/bcachefs/xattr.c
> @@ -38,7 +38,7 @@ static u64 xattr_hash_bkey(const struct bch_hash_info *info, struct bkey_s_c k)
>  	struct bkey_s_c_xattr x = bkey_s_c_to_xattr(k);
>  
>  	return bch2_xattr_hash(info,
> -		 &X_SEARCH(x.v->x_type, x.v->x_name, x.v->x_name_len));
> +		 &X_SEARCH(x.v->x_type, x.v->x_name_and_value, x.v->x_name_len));
>  }
>  
>  static bool xattr_cmp_key(struct bkey_s_c _l, const void *_r)
> @@ -48,7 +48,7 @@ static bool xattr_cmp_key(struct bkey_s_c _l, const void *_r)
>  
>  	return l.v->x_type != r->type ||
>  		l.v->x_name_len != r->name.len ||
> -		memcmp(l.v->x_name, r->name.name, r->name.len);
> +		memcmp(l.v->x_name_and_value, r->name.name, r->name.len);
>  }
>  
>  static bool xattr_cmp_bkey(struct bkey_s_c _l, struct bkey_s_c _r)
> @@ -58,7 +58,7 @@ static bool xattr_cmp_bkey(struct bkey_s_c _l, struct bkey_s_c _r)
>  
>  	return l.v->x_type != r.v->x_type ||
>  		l.v->x_name_len != r.v->x_name_len ||
> -		memcmp(l.v->x_name, r.v->x_name, r.v->x_name_len);
> +		memcmp(l.v->x_name_and_value, r.v->x_name_and_value, r.v->x_name_len);
>  }
>  
>  const struct bch_hash_desc bch2_xattr_hash_desc = {
> @@ -96,7 +96,7 @@ int bch2_xattr_validate(struct bch_fs *c, struct bkey_s_c k,
>  			 c, xattr_invalid_type,
>  			 "invalid type (%u)", xattr.v->x_type);
>  
> -	bkey_fsck_err_on(memchr(xattr.v->x_name, '\0', xattr.v->x_name_len),
> +	bkey_fsck_err_on(memchr(xattr.v->x_name_and_value, '\0', xattr.v->x_name_len),
>  			 c, xattr_name_invalid_chars,
>  			 "xattr name has invalid characters");
>  fsck_err:
> @@ -120,13 +120,13 @@ void bch2_xattr_to_text(struct printbuf *out, struct bch_fs *c,
>  	unsigned name_len = xattr.v->x_name_len;
>  	unsigned val_len  = le16_to_cpu(xattr.v->x_val_len);
>  	unsigned max_name_val_bytes = bkey_val_bytes(xattr.k) -
> -		offsetof(struct bch_xattr, x_name);
> +		offsetof(struct bch_xattr, x_name_and_value);
>  
>  	val_len  = min_t(int, val_len, max_name_val_bytes - name_len);
>  	name_len = min(name_len, max_name_val_bytes);
>  
>  	prt_printf(out, "%.*s:%.*s",
> -		   name_len, xattr.v->x_name,
> +		   name_len, xattr.v->x_name_and_value,
>  		   val_len,  (char *) xattr_val(xattr.v));
>  
>  	if (xattr.v->x_type == KEY_TYPE_XATTR_INDEX_POSIX_ACL_ACCESS ||
> @@ -202,7 +202,7 @@ int bch2_xattr_set(struct btree_trans *trans, subvol_inum inum,
>  		xattr->v.x_type		= type;
>  		xattr->v.x_name_len	= namelen;
>  		xattr->v.x_val_len	= cpu_to_le16(size);
> -		memcpy(xattr->v.x_name, name, namelen);
> +		memcpy(xattr->v.x_name_and_value, name, namelen);
>  		memcpy(xattr_val(&xattr->v), value, size);
>  
>  		ret = bch2_hash_set(trans, bch2_xattr_hash_desc, hash_info,
> @@ -270,7 +270,7 @@ static int bch2_xattr_emit(struct dentry *dentry,
>  	if (!prefix)
>  		return 0;
>  
> -	return __bch2_xattr_emit(prefix, xattr->x_name, xattr->x_name_len, buf);
> +	return __bch2_xattr_emit(prefix, xattr->x_name_and_value, xattr->x_name_len, buf);
>  }
>  
>  static int bch2_xattr_list_bcachefs(struct bch_fs *c,
> diff --git a/fs/bcachefs/xattr.h b/fs/bcachefs/xattr.h
> index 132fbbd15a66..1139bf345f70 100644
> --- a/fs/bcachefs/xattr.h
> +++ b/fs/bcachefs/xattr.h
> @@ -18,12 +18,12 @@ void bch2_xattr_to_text(struct printbuf *, struct bch_fs *, struct bkey_s_c);
>  
>  static inline unsigned xattr_val_u64s(unsigned name_len, unsigned val_len)
>  {
> -	return DIV_ROUND_UP(offsetof(struct bch_xattr, x_name) +
> +	return DIV_ROUND_UP(offsetof(struct bch_xattr, x_name_and_value) +
>  			    name_len + val_len, sizeof(u64));
>  }
>  
>  #define xattr_val(_xattr)					\
> -	((void *) (_xattr)->x_name + (_xattr)->x_name_len)
> +	((void *) (_xattr)->x_name_and_value + (_xattr)->x_name_len)
>  
>  struct xattr_search_key {
>  	u8		type;
> diff --git a/fs/bcachefs/xattr_format.h b/fs/bcachefs/xattr_format.h
> index 67426e33d04e..4121b78d9a92 100644
> --- a/fs/bcachefs/xattr_format.h
> +++ b/fs/bcachefs/xattr_format.h
> @@ -16,10 +16,10 @@ struct bch_xattr {
>  	/*
>  	 * x_name contains the name and value counted by
>  	 * x_name_len + x_val_len. The introduction of
> -	 * __counted_by(x_name_len) caused a false positive
> +	 * __counted_by(x_name_len) previously caused a false positive
>  	 * detection of an out of bounds write.
>  	 */
> -	__u8			x_name[];
> +	__u8			x_name_and_value[];
>  } __packed __aligned(8);
>  
>  #endif /* _BCACHEFS_XATTR_FORMAT_H */
> -- 
> 2.48.1
> 

Re: [PATCH 1/2] bcachefs: Remove incorrect __counted_by annotation

[Thorsten Blum]

On 1. May 2025, at 22:01, Alan Huang wrote:
> +	/*
> +	 * x_name contains the name and value counted by
> +	 * x_name_len + x_val_len. The introduction of
> +	 * __counted_by(x_name_len) caused a false positive
> +	 * detection of an out of bounds write.
> +	 */

Instead of removing it, would __counted_by(x_name_len + x_val_len) work?

Best,
Thorsten

Re: [PATCH 1/2] bcachefs: Remove incorrect __counted_by annotation

[Thorsten Blum]

On 1. May 2025, at 22:08, Thorsten Blum wrote:
> Instead of removing it, would __counted_by(x_name_len + x_val_len) work?

Ah sorry, never mind, that obviously doesn't work.

Re: [PATCH 1/2] bcachefs: Remove incorrect __counted_by annotation

[Kees Cook]

On Thu, May 01, 2025 at 10:08:16PM +0200, Thorsten Blum wrote:
> On 1. May 2025, at 22:01, Alan Huang wrote:
> > +	/*
> > +	 * x_name contains the name and value counted by
> > +	 * x_name_len + x_val_len. The introduction of
> > +	 * __counted_by(x_name_len) caused a false positive
> > +	 * detection of an out of bounds write.
> > +	 */
> 
> Instead of removing it, would __counted_by(x_name_len + x_val_len) work?

Not yet, but once __counted_by_expr() has landed in GCC and Clang, yes
it will. :)

-- 
Kees Cook

Re: [PATCH 1/2] bcachefs: Remove incorrect __counted_by annotation

[Jan Hendrik Farr]

On 02 04:01:31, Alan Huang wrote:
> This actually reverts 86e92eeeb237 ("bcachefs: Annotate struct bch_xattr
> with __counted_by()").
> 
> After the x_name, there is a value. According to the disscussion[1],
> __counted_by assumes that the flexible array member contains exactly
> the amount of elements that are specified. Now there are users came across
> a false positive detection of an out of bounds write caused by
> the __counted_by here[2], so revert that.
> 
> [1] https://lore.kernel.org/lkml/Zv8VDKWN1GzLRT-_@archlinux/T/#m0ce9541c5070146320efd4f928cc1ff8de69e9b2
> [2] https://privatebin.net/?a0d4e97d590d71e1#9bLmp2Kb5NU6X6cZEucchDcu88HzUQwHUah8okKPReEt
> 
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>

Reviewed-by: Jan Hendrik Farr <kernel@jfarr.cc>

> ---
>  fs/bcachefs/xattr_format.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/xattr_format.h b/fs/bcachefs/xattr_format.h
> index c7916011ef34..67426e33d04e 100644
> --- a/fs/bcachefs/xattr_format.h
> +++ b/fs/bcachefs/xattr_format.h
> @@ -13,7 +13,13 @@ struct bch_xattr {
>  	__u8			x_type;
>  	__u8			x_name_len;
>  	__le16			x_val_len;
> -	__u8			x_name[] __counted_by(x_name_len);
> +	/*
> +	 * x_name contains the name and value counted by
> +	 * x_name_len + x_val_len. The introduction of
> +	 * __counted_by(x_name_len) caused a false positive
> +	 * detection of an out of bounds write.
> +	 */

In my estimation the comment isn't strictly needed with the name
change in Patch 2/2, but it can also stay.

> +	__u8			x_name[];
>  } __packed __aligned(8);
>  
>  #endif /* _BCACHEFS_XATTR_FORMAT_H */
> -- 
> 2.48.1
> 

I was able to reproduce this issue with gcc 15.1.1 and bcachefs as
rootfs (and verify that this fixes it), but wasn't able to reproduce
using clang 19.1.7. Turns out there is one more difference in how
gcc and clang do __bdos. clang apparantly doesn't handle pointer
arithmetic done using + symbol instead of [] for the __counted_by
case. Here's a reproducer:
https://godbolt.org/z/136T98Wdz

Best Regards
Jan