linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc
@ 2025-09-19 14:58 Miquel Sabaté Solà
  2025-09-19 14:58 ` [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc Miquel Sabaté Solà
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-19 14:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: clm, dsterba, linux-kernel, Miquel Sabaté Solà

Hello,

There were a couple of instances in btrfs code in which kmalloc was being
used with open-coded arithmetic. This can lead into unfortunate overflow
situations as describbed here [1]. The solution is to use kmalloc_array in
these cases, which is what it's being done in my first patch.

The second patch is a small cleanup after fixing up my first patch, in
which I realized that the __free(kfree) attribute would come in handy in a
couple of particularly large functions with multiple exit points. This
second patch is probably more of a cosmetic thing, and it's not an
exhaustive exercise by any means. All of this to say that even if I feel
like it should be included, I don't mind if it has to be dropped.

Cheers,
Miquel

[1] Documentation/process/deprecated.rst

Miquel Sabaté Solà (2):
  btrfs: Prevent open-coded arithmetic in kmalloc
  btrfs: Prefer using the __free cleanup attribute

 fs/btrfs/delayed-inode.c | 18 ++++++++----------
 fs/btrfs/tree-log.c      | 30 +++++++++++-------------------
 2 files changed, 19 insertions(+), 29 deletions(-)

--
2.51.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc
  2025-09-19 14:58 [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc Miquel Sabaté Solà
@ 2025-09-19 14:58 ` Miquel Sabaté Solà
  2025-09-22 10:28   ` David Sterba
  2025-09-19 14:58 ` [PATCH 2/2] btrfs: Prefer using the __free cleanup attribute Miquel Sabaté Solà
  2025-09-22 10:34 ` [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc David Sterba
  2 siblings, 1 reply; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-19 14:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: clm, dsterba, linux-kernel, Miquel Sabaté Solà

As pointed out in the documentation, calling 'kmalloc' with open-coded
arithmetic can lead to unfortunate overflows and this particular way of
using it has been deprecated. Instead, it's preferred to use
'kmalloc_array' in cases where it might apply so an overflow check is
performed.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/btrfs/delayed-inode.c | 4 ++--
 fs/btrfs/tree-log.c      | 9 +++------
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 6adfe62cd0c4..81577a0c601f 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -738,8 +738,8 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans,
 		u32 *ins_sizes;
 		int i = 0;
 
-		ins_data = kmalloc(batch.nr * sizeof(u32) +
-				   batch.nr * sizeof(struct btrfs_key), GFP_NOFS);
+		ins_data = kmalloc_array(batch.nr,
+					 sizeof(u32) + sizeof(struct btrfs_key), GFP_NOFS);
 		if (!ins_data) {
 			ret = -ENOMEM;
 			goto out;
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 7d19a8c5b2a3..d6471cd33f7f 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4062,8 +4062,7 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
 		struct btrfs_key *ins_keys;
 		u32 *ins_sizes;
 
-		ins_data = kmalloc(count * sizeof(u32) +
-				   count * sizeof(struct btrfs_key), GFP_NOFS);
+		ins_data = kmalloc_array(count, sizeof(u32) + sizeof(struct btrfs_key), GFP_NOFS);
 		if (!ins_data)
 			return -ENOMEM;
 
@@ -4826,8 +4825,7 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
 
 	src = src_path->nodes[0];
 
-	ins_data = kmalloc(nr * sizeof(struct btrfs_key) +
-			   nr * sizeof(u32), GFP_NOFS);
+	ins_data = kmalloc_array(nr, sizeof(struct btrfs_key) + sizeof(u32), GFP_NOFS);
 	if (!ins_data)
 		return -ENOMEM;
 
@@ -6532,8 +6530,7 @@ static int log_delayed_insertion_items(struct btrfs_trans_handle *trans,
 	if (!first)
 		return 0;
 
-	ins_data = kmalloc(max_batch_size * sizeof(u32) +
-			   max_batch_size * sizeof(struct btrfs_key), GFP_NOFS);
+	ins_data = kmalloc_array(max_batch_size, sizeof(u32) + sizeof(struct btrfs_key), GFP_NOFS);
 	if (!ins_data)
 		return -ENOMEM;
 	ins_sizes = (u32 *)ins_data;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] btrfs: Prefer using the __free cleanup attribute
  2025-09-19 14:58 [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc Miquel Sabaté Solà
  2025-09-19 14:58 ` [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc Miquel Sabaté Solà
@ 2025-09-19 14:58 ` Miquel Sabaté Solà
  2025-09-22 10:34 ` [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc David Sterba
  2 siblings, 0 replies; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-19 14:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: clm, dsterba, linux-kernel, Miquel Sabaté Solà

In delayed-node.c and tree-log.c there were a couple of instances where
the __free(kfree) cleanup attribute allowed for more clear code and
safer for future changes, as they were in large functions with multiple
exit points and the end cleanup code was simply calling 'kfree' for the
ins_data variable.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/btrfs/delayed-inode.c | 14 ++++++--------
 fs/btrfs/tree-log.c      | 21 ++++++++-------------
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 81577a0c601f..dbc63dc414bb 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -668,7 +668,7 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans,
 	struct btrfs_key first_key;
 	const u32 first_data_size = first_item->data_len;
 	int total_size;
-	char *ins_data = NULL;
+	char *ins_data __free(kfree) = NULL;
 	int ret;
 	bool continuous_keys_only = false;
 
@@ -740,10 +740,9 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans,
 
 		ins_data = kmalloc_array(batch.nr,
 					 sizeof(u32) + sizeof(struct btrfs_key), GFP_NOFS);
-		if (!ins_data) {
-			ret = -ENOMEM;
-			goto out;
-		}
+		if (!ins_data)
+			return -ENOMEM;
+
 		ins_sizes = (u32 *)ins_data;
 		ins_keys = (struct btrfs_key *)(ins_data + batch.nr * sizeof(u32));
 		batch.keys = ins_keys;
@@ -759,7 +758,7 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_insert_empty_items(trans, root, path, &batch);
 	if (ret)
-		goto out;
+		return ret;
 
 	list_for_each_entry(curr, &item_list, tree_list) {
 		char *data_ptr;
@@ -814,8 +813,7 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans,
 		list_del(&curr->tree_list);
 		btrfs_release_delayed_item(curr);
 	}
-out:
-	kfree(ins_data);
+
 	return ret;
 }
 
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index d6471cd33f7f..c376514cf844 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3302,7 +3302,7 @@ static inline void btrfs_remove_log_ctx(struct btrfs_root *root,
 	mutex_unlock(&root->log_mutex);
 }
 
-/* 
+/*
  * Invoked in log mutex context, or be sure there is no other task which
  * can access the list.
  */
@@ -4038,7 +4038,7 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
 				 int count)
 {
 	struct btrfs_root *log = inode->root->log_root;
-	char *ins_data = NULL;
+	char *ins_data __free(kfree) = NULL;
 	struct btrfs_item_batch batch;
 	struct extent_buffer *dst;
 	unsigned long src_offset;
@@ -4083,7 +4083,7 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_insert_empty_items(trans, log, dst_path, &batch);
 	if (ret)
-		goto out;
+		return ret;
 
 	dst = dst_path->nodes[0];
 	/*
@@ -4115,8 +4115,6 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
 
 	if (btrfs_get_first_dir_index_to_log(inode) == 0)
 		btrfs_set_first_dir_index_to_log(inode, batch.keys[0].offset);
-out:
-	kfree(ins_data);
 
 	return ret;
 }
@@ -4786,7 +4784,7 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
 	struct btrfs_key *ins_keys;
 	u32 *ins_sizes;
 	struct btrfs_item_batch batch;
-	char *ins_data;
+	char *ins_data __free(kfree) = NULL;
 	int dst_index;
 	const bool skip_csum = (inode->flags & BTRFS_INODE_NODATASUM);
 	const u64 i_size = i_size_read(&inode->vfs_inode);
@@ -4914,7 +4912,7 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
 					      disk_bytenr + extent_num_bytes - 1,
 					      &ordered_sums, false);
 		if (ret < 0)
-			goto out;
+			return ret;
 		ret = 0;
 
 		list_for_each_entry_safe(sums, sums_next, &ordered_sums, list) {
@@ -4924,7 +4922,7 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
 			kfree(sums);
 		}
 		if (ret)
-			goto out;
+			return ret;
 
 add_to_batch:
 		ins_sizes[dst_index] = btrfs_item_size(src, src_slot);
@@ -4938,11 +4936,11 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
 	 * so we don't need to do anything.
 	 */
 	if (batch.nr == 0)
-		goto out;
+		return 0;
 
 	ret = btrfs_insert_empty_items(trans, log, dst_path, &batch);
 	if (ret)
-		goto out;
+		return ret;
 
 	dst_index = 0;
 	for (int i = 0; i < nr; i++) {
@@ -4995,8 +4993,6 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
 	}
 
 	btrfs_release_path(dst_path);
-out:
-	kfree(ins_data);
 
 	return ret;
 }
@@ -8082,4 +8078,3 @@ void btrfs_log_new_name(struct btrfs_trans_handle *trans,
 		btrfs_end_log_trans(root);
 	free_extent_buffer(ctx.scratch_eb);
 }
-
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc
  2025-09-19 14:58 ` [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc Miquel Sabaté Solà
@ 2025-09-22 10:28   ` David Sterba
  2025-09-22 12:47     ` Miquel Sabaté Solà
  0 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2025-09-22 10:28 UTC (permalink / raw)
  To: Miquel Sabaté Solà; +Cc: linux-btrfs, clm, dsterba, linux-kernel

On Fri, Sep 19, 2025 at 04:58:15PM +0200, Miquel Sabaté Solà wrote:
> As pointed out in the documentation, calling 'kmalloc' with open-coded
> arithmetic can lead to unfortunate overflows and this particular way of
> using it has been deprecated. Instead, it's preferred to use
> 'kmalloc_array' in cases where it might apply so an overflow check is
> performed.

So this is an API cleanup and it makes sense to use the checked
multiplication but it should be also said that this is not fixing any
overflow because in all cases the multipliers are bounded small numbers
derived from number of items in leaves/nodes.

> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>

Reviewed-by: David Sterba <dsterba@suse.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc
  2025-09-19 14:58 [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc Miquel Sabaté Solà
  2025-09-19 14:58 ` [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc Miquel Sabaté Solà
  2025-09-19 14:58 ` [PATCH 2/2] btrfs: Prefer using the __free cleanup attribute Miquel Sabaté Solà
@ 2025-09-22 10:34 ` David Sterba
  2025-09-22 12:51   ` Miquel Sabaté Solà
  2 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2025-09-22 10:34 UTC (permalink / raw)
  To: Miquel Sabaté Solà; +Cc: linux-btrfs, clm, dsterba, linux-kernel

On Fri, Sep 19, 2025 at 04:58:14PM +0200, Miquel Sabaté Solà wrote:
> The second patch is a small cleanup after fixing up my first patch, in
> which I realized that the __free(kfree) attribute would come in handy in a
> couple of particularly large functions with multiple exit points. This
> second patch is probably more of a cosmetic thing, and it's not an
> exhaustive exercise by any means. All of this to say that even if I feel
> like it should be included, I don't mind if it has to be dropped.

Yes there are many candidates for the __free() cleanup annotation and
we'll want to fix them all systematically. We already have the automatic
cleaning for struct btrfs_path (BTRFS_PATH_AUTO_FREE). For the
kfree/kvfree I'd like to something similar:

#define AUTO_KFREE(name)       *name __free(kfree) = NULL
#define AUTO_KVFREE(name)      *name __free(kvfree) = NULL

This wraps the name and initializes it to NULL so it's not accidentally
forgotten.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc
  2025-09-22 10:28   ` David Sterba
@ 2025-09-22 12:47     ` Miquel Sabaté Solà
  2025-09-23  6:13       ` David Sterba
  0 siblings, 1 reply; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-22 12:47 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs, clm, dsterba, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

Hello,

David Sterba @ 2025-09-22 12:28 +02:

> On Fri, Sep 19, 2025 at 04:58:15PM +0200, Miquel Sabaté Solà wrote:
>> As pointed out in the documentation, calling 'kmalloc' with open-coded
>> arithmetic can lead to unfortunate overflows and this particular way of
>> using it has been deprecated. Instead, it's preferred to use
>> 'kmalloc_array' in cases where it might apply so an overflow check is
>> performed.
>
> So this is an API cleanup and it makes sense to use the checked
> multiplication but it should be also said that this is not fixing any
> overflow because in all cases the multipliers are bounded small numbers
> derived from number of items in leaves/nodes.

Yes, it's just an API cleanup and I don't think it fixes any current bug
in the code base. So no need to CC stable or anything like that.

>
>> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
>
> Reviewed-by: David Sterba <dsterba@suse.com>

Thanks for the review!
Miquel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc
  2025-09-22 10:34 ` [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc David Sterba
@ 2025-09-22 12:51   ` Miquel Sabaté Solà
  2025-09-23  6:11     ` David Sterba
  0 siblings, 1 reply; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-22 12:51 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs, clm, dsterba, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

Hello,

David Sterba @ 2025-09-22 12:34 +02:

> On Fri, Sep 19, 2025 at 04:58:14PM +0200, Miquel Sabaté Solà wrote:
>> The second patch is a small cleanup after fixing up my first patch, in
>> which I realized that the __free(kfree) attribute would come in handy in a
>> couple of particularly large functions with multiple exit points. This
>> second patch is probably more of a cosmetic thing, and it's not an
>> exhaustive exercise by any means. All of this to say that even if I feel
>> like it should be included, I don't mind if it has to be dropped.
>
> Yes there are many candidates for the __free() cleanup annotation and
> we'll want to fix them all systematically. We already have the automatic
> cleaning for struct btrfs_path (BTRFS_PATH_AUTO_FREE). For the
> kfree/kvfree I'd like to something similar:
>
> #define AUTO_KFREE(name)       *name __free(kfree) = NULL
> #define AUTO_KVFREE(name)      *name __free(kvfree) = NULL
>
> This wraps the name and initializes it to NULL so it's not accidentally
> forgotten.

Makes sense! I can take a look at this if nobody else is working on it,
even if I think it should go into a new patch series.

Hence, if it sounds good to you, we can merge this patch as it is right
now, and in parallel I work on this proposed AUTO_KFREE and AUTO_KVFREE
macros in a new patch series (which will take more time to prepare).

Thanks,
Miquel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc
  2025-09-22 12:51   ` Miquel Sabaté Solà
@ 2025-09-23  6:11     ` David Sterba
  2025-09-23  6:46       ` Miquel Sabaté Solà
  0 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2025-09-23  6:11 UTC (permalink / raw)
  To: Miquel Sabaté Solà
  Cc: David Sterba, linux-btrfs, clm, dsterba, linux-kernel

On Mon, Sep 22, 2025 at 02:51:15PM +0200, Miquel Sabaté Solà wrote:
> > On Fri, Sep 19, 2025 at 04:58:14PM +0200, Miquel Sabaté Solà wrote:
> >> The second patch is a small cleanup after fixing up my first patch, in
> >> which I realized that the __free(kfree) attribute would come in handy in a
> >> couple of particularly large functions with multiple exit points. This
> >> second patch is probably more of a cosmetic thing, and it's not an
> >> exhaustive exercise by any means. All of this to say that even if I feel
> >> like it should be included, I don't mind if it has to be dropped.
> >
> > Yes there are many candidates for the __free() cleanup annotation and
> > we'll want to fix them all systematically. We already have the automatic
> > cleaning for struct btrfs_path (BTRFS_PATH_AUTO_FREE). For the
> > kfree/kvfree I'd like to something similar:
> >
> > #define AUTO_KFREE(name)       *name __free(kfree) = NULL
> > #define AUTO_KVFREE(name)      *name __free(kvfree) = NULL
> >
> > This wraps the name and initializes it to NULL so it's not accidentally
> > forgotten.
> 
> Makes sense! I can take a look at this if nobody else is working on it,
> even if I think it should go into a new patch series.

Thanks, it's yours. Yes this should be in a separate patchset.

> Hence, if it sounds good to you, we can merge this patch as it is right
> now, and in parallel I work on this proposed AUTO_KFREE and AUTO_KVFREE
> macros in a new patch series (which will take more time to prepare).

I'd rather see all the changes done the same way so it's not __free and
then converted to AUTO_KFREE. Also the development branch is frozen
before 6.18 pull request so all that will be in the 6.19 cycle anyway.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc
  2025-09-22 12:47     ` Miquel Sabaté Solà
@ 2025-09-23  6:13       ` David Sterba
  2025-09-23  6:47         ` Miquel Sabaté Solà
  0 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2025-09-23  6:13 UTC (permalink / raw)
  To: Miquel Sabaté Solà
  Cc: David Sterba, linux-btrfs, clm, dsterba, linux-kernel

On Mon, Sep 22, 2025 at 02:47:13PM +0200, Miquel Sabaté Solà wrote:
> Hello,
> 
> David Sterba @ 2025-09-22 12:28 +02:
> 
> > On Fri, Sep 19, 2025 at 04:58:15PM +0200, Miquel Sabaté Solà wrote:
> >> As pointed out in the documentation, calling 'kmalloc' with open-coded
> >> arithmetic can lead to unfortunate overflows and this particular way of
> >> using it has been deprecated. Instead, it's preferred to use
> >> 'kmalloc_array' in cases where it might apply so an overflow check is
> >> performed.
> >
> > So this is an API cleanup and it makes sense to use the checked
> > multiplication but it should be also said that this is not fixing any
> > overflow because in all cases the multipliers are bounded small numbers
> > derived from number of items in leaves/nodes.
> 
> Yes, it's just an API cleanup and I don't think it fixes any current bug
> in the code base. So no need to CC stable or anything like that.

Still the changelog should say explicitly that it's not a bug fix before
somebody assigns a CVE to it because it mentions overflow.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc
  2025-09-23  6:11     ` David Sterba
@ 2025-09-23  6:46       ` Miquel Sabaté Solà
  0 siblings, 0 replies; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-23  6:46 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs, clm, dsterba, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

David Sterba @ 2025-09-23 08:11 +02:

> On Mon, Sep 22, 2025 at 02:51:15PM +0200, Miquel Sabaté Solà wrote:
>> > On Fri, Sep 19, 2025 at 04:58:14PM +0200, Miquel Sabaté Solà wrote:
>> >> The second patch is a small cleanup after fixing up my first patch, in
>> >> which I realized that the __free(kfree) attribute would come in handy in a
>> >> couple of particularly large functions with multiple exit points. This
>> >> second patch is probably more of a cosmetic thing, and it's not an
>> >> exhaustive exercise by any means. All of this to say that even if I feel
>> >> like it should be included, I don't mind if it has to be dropped.
>> >
>> > Yes there are many candidates for the __free() cleanup annotation and
>> > we'll want to fix them all systematically. We already have the automatic
>> > cleaning for struct btrfs_path (BTRFS_PATH_AUTO_FREE). For the
>> > kfree/kvfree I'd like to something similar:
>> >
>> > #define AUTO_KFREE(name)       *name __free(kfree) = NULL
>> > #define AUTO_KVFREE(name)      *name __free(kvfree) = NULL
>> >
>> > This wraps the name and initializes it to NULL so it's not accidentally
>> > forgotten.
>>
>> Makes sense! I can take a look at this if nobody else is working on it,
>> even if I think it should go into a new patch series.
>
> Thanks, it's yours. Yes this should be in a separate patchset.

Great, will do!

>
>> Hence, if it sounds good to you, we can merge this patch as it is right
>> now, and in parallel I work on this proposed AUTO_KFREE and AUTO_KVFREE
>> macros in a new patch series (which will take more time to prepare).
>
> I'd rather see all the changes done the same way so it's not __free and
> then converted to AUTO_KFREE. Also the development branch is frozen
> before 6.18 pull request so all that will be in the 6.19 cycle anyway.

Got it. Then in v2 I will drop this in favor of the later patchset.

Greetings,
Miquel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc
  2025-09-23  6:13       ` David Sterba
@ 2025-09-23  6:47         ` Miquel Sabaté Solà
  2025-09-23  7:00           ` David Sterba
  0 siblings, 1 reply; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-23  6:47 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs, clm, dsterba, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

David Sterba @ 2025-09-23 08:13 +02:

> On Mon, Sep 22, 2025 at 02:47:13PM +0200, Miquel Sabaté Solà wrote:
>> Hello,
>>
>> David Sterba @ 2025-09-22 12:28 +02:
>>
>> > On Fri, Sep 19, 2025 at 04:58:15PM +0200, Miquel Sabaté Solà wrote:
>> >> As pointed out in the documentation, calling 'kmalloc' with open-coded
>> >> arithmetic can lead to unfortunate overflows and this particular way of
>> >> using it has been deprecated. Instead, it's preferred to use
>> >> 'kmalloc_array' in cases where it might apply so an overflow check is
>> >> performed.
>> >
>> > So this is an API cleanup and it makes sense to use the checked
>> > multiplication but it should be also said that this is not fixing any
>> > overflow because in all cases the multipliers are bounded small numbers
>> > derived from number of items in leaves/nodes.
>>
>> Yes, it's just an API cleanup and I don't think it fixes any current bug
>> in the code base. So no need to CC stable or anything like that.
>
> Still the changelog should say explicitly that it's not a bug fix before
> somebody assigns a CVE to it because it mentions overflow.

Got it! I will submit a v2 and make this more explicit.

Thanks,
Miquel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc
  2025-09-23  6:47         ` Miquel Sabaté Solà
@ 2025-09-23  7:00           ` David Sterba
  2025-09-23  8:00             ` Miquel Sabaté Solà
  0 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2025-09-23  7:00 UTC (permalink / raw)
  To: Miquel Sabaté Solà
  Cc: David Sterba, linux-btrfs, clm, dsterba, linux-kernel

On Tue, Sep 23, 2025 at 08:47:35AM +0200, Miquel Sabaté Solà wrote:
> David Sterba @ 2025-09-23 08:13 +02:
> 
> > On Mon, Sep 22, 2025 at 02:47:13PM +0200, Miquel Sabaté Solà wrote:
> >> Hello,
> >>
> >> David Sterba @ 2025-09-22 12:28 +02:
> >>
> >> > On Fri, Sep 19, 2025 at 04:58:15PM +0200, Miquel Sabaté Solà wrote:
> >> >> As pointed out in the documentation, calling 'kmalloc' with open-coded
> >> >> arithmetic can lead to unfortunate overflows and this particular way of
> >> >> using it has been deprecated. Instead, it's preferred to use
> >> >> 'kmalloc_array' in cases where it might apply so an overflow check is
> >> >> performed.
> >> >
> >> > So this is an API cleanup and it makes sense to use the checked
> >> > multiplication but it should be also said that this is not fixing any
> >> > overflow because in all cases the multipliers are bounded small numbers
> >> > derived from number of items in leaves/nodes.
> >>
> >> Yes, it's just an API cleanup and I don't think it fixes any current bug
> >> in the code base. So no need to CC stable or anything like that.
> >
> > Still the changelog should say explicitly that it's not a bug fix before
> > somebody assigns a CVE to it because it mentions overflow.
> 
> Got it! I will submit a v2 and make this more explicit.

No need to, I've updated the changelog at commit time.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc
  2025-09-23  7:00           ` David Sterba
@ 2025-09-23  8:00             ` Miquel Sabaté Solà
  0 siblings, 0 replies; 13+ messages in thread
From: Miquel Sabaté Solà @ 2025-09-23  8:00 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs, clm, dsterba, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

David Sterba @ 2025-09-23 09:00 +02:

> On Tue, Sep 23, 2025 at 08:47:35AM +0200, Miquel Sabaté Solà wrote:
>> David Sterba @ 2025-09-23 08:13 +02:
>>
>> > On Mon, Sep 22, 2025 at 02:47:13PM +0200, Miquel Sabaté Solà wrote:
>> >> Hello,
>> >>
>> >> David Sterba @ 2025-09-22 12:28 +02:
>> >>
>> >> > On Fri, Sep 19, 2025 at 04:58:15PM +0200, Miquel Sabaté Solà wrote:
>> >> >> As pointed out in the documentation, calling 'kmalloc' with open-coded
>> >> >> arithmetic can lead to unfortunate overflows and this particular way of
>> >> >> using it has been deprecated. Instead, it's preferred to use
>> >> >> 'kmalloc_array' in cases where it might apply so an overflow check is
>> >> >> performed.
>> >> >
>> >> > So this is an API cleanup and it makes sense to use the checked
>> >> > multiplication but it should be also said that this is not fixing any
>> >> > overflow because in all cases the multipliers are bounded small numbers
>> >> > derived from number of items in leaves/nodes.
>> >>
>> >> Yes, it's just an API cleanup and I don't think it fixes any current bug
>> >> in the code base. So no need to CC stable or anything like that.
>> >
>> > Still the changelog should say explicitly that it's not a bug fix before
>> > somebody assigns a CVE to it because it mentions overflow.
>>
>> Got it! I will submit a v2 and make this more explicit.
>
> No need to, I've updated the changelog at commit time.

Ah, then ignore the v2 patch set I've just sent, which simply changes
the commit log as you suggested.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-09-23  8:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-19 14:58 [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc Miquel Sabaté Solà
2025-09-19 14:58 ` [PATCH 1/2] btrfs: Prevent open-coded arithmetic in kmalloc Miquel Sabaté Solà
2025-09-22 10:28   ` David Sterba
2025-09-22 12:47     ` Miquel Sabaté Solà
2025-09-23  6:13       ` David Sterba
2025-09-23  6:47         ` Miquel Sabaté Solà
2025-09-23  7:00           ` David Sterba
2025-09-23  8:00             ` Miquel Sabaté Solà
2025-09-19 14:58 ` [PATCH 2/2] btrfs: Prefer using the __free cleanup attribute Miquel Sabaté Solà
2025-09-22 10:34 ` [PATCH 0/2] btrfs: Prevent open-coded arithmetic on kmalloc David Sterba
2025-09-22 12:51   ` Miquel Sabaté Solà
2025-09-23  6:11     ` David Sterba
2025-09-23  6:46       ` Miquel Sabaté Solà

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).