fs/btrfs/delayed-inode.c | 18 ++++++++---------- fs/btrfs/tree-log.c | 30 +++++++++++------------------- 2 files changed, 19 insertions(+), 29 deletions(-)
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
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.
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
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.
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
© 2016 - 2025 Red Hat, Inc.