| Message ID | ZgSrBVidW1U6yP+h@ayush-HP-Pavilion-Gaming-Laptop-15-ec0xxx (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | LANDLOCK: use kmem_cache for landlock_object | expand |
On Wed, Mar 27, 2024 at 7:26 PM Ayush Tiwari <ayushtiw0110@gmail.com> wrote: > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > struct landlock_object and update the related dependencies. > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> > --- > security/landlock/fs.c | 2 +- > security/landlock/object.c | 14 ++++++++++++-- > security/landlock/object.h | 4 ++++ > security/landlock/setup.c | 2 ++ > 4 files changed, 19 insertions(+), 3 deletions(-) Hi Ayush, Mickaël has the final say on Landlock patches, but I had a few comments that I've included below ... > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index fc520a06f9af..227dd67dd902 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -124,7 +124,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode) > if (unlikely(rcu_access_pointer(inode_sec->object))) { > /* Someone else just created the object, bail out and retry. */ > spin_unlock(&inode->i_lock); > - kfree(new_object); > + kmem_cache_free(landlock_object_cache, new_object); See my comment below, but you may want to wrap this in a Landlock object API function. > rcu_read_lock(); > goto retry; > diff --git a/security/landlock/object.c b/security/landlock/object.c > index 1f50612f0185..df1354215617 100644 > --- a/security/landlock/object.c > +++ b/security/landlock/object.c > @@ -17,6 +17,15 @@ > > #include "object.h" > > +struct kmem_cache *landlock_object_cache; > + > +void __init landlock_object_init(void) > +{ > + landlock_object_cache = kmem_cache_create( > + "landlock_object_cache", sizeof(struct landlock_object), 0, > + SLAB_PANIC, NULL); The comments in include/linux/slab.h suggest using the KMEM_CACHE() macro, instead of kmem_cache_create(), as a best practice for creating slab caches. > +} > + > struct landlock_object * > landlock_create_object(const struct landlock_object_underops *const underops, > void *const underobj) > @@ -25,7 +34,8 @@ landlock_create_object(const struct landlock_object_underops *const underops, > > if (WARN_ON_ONCE(!underops || !underobj)) > return ERR_PTR(-ENOENT); > - new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT); > + new_object = > + kmem_cache_zalloc(landlock_object_cache, GFP_KERNEL_ACCOUNT); If the line is too long, you might want to consider splitting the function parameters like this: new_object = kmem_cache_zalloc(landlock_object_cache, GFP_KERNEL_ACCOUNT); > if (!new_object) > return ERR_PTR(-ENOMEM); > refcount_set(&new_object->usage, 1); > @@ -62,6 +72,6 @@ void landlock_put_object(struct landlock_object *const object) > * @object->underobj to @object (if it still exists). > */ > object->underops->release(object); > - kfree_rcu(object, rcu_free); > + kmem_cache_free(landlock_object_cache, object); > } > } > diff --git a/security/landlock/object.h b/security/landlock/object.h > index 5f28c35e8aa8..8ba1af3ddc2e 100644 > --- a/security/landlock/object.h > +++ b/security/landlock/object.h > @@ -13,6 +13,10 @@ > #include <linux/refcount.h> > #include <linux/spinlock.h> > > +extern struct kmem_cache *landlock_object_cache; This really is a decision for Mickaël, but you may want to make @landlock_object_cache private to object.c and create functions to manage it as needed, e.g. put/free operations. > +void __init landlock_object_init(void); > + > struct landlock_object; > > /** > diff --git a/security/landlock/setup.c b/security/landlock/setup.c > index f6dd33143b7f..a5fca4582ee1 100644
On Thu, Mar 28, 2024 at 04:55:57AM +0530, Ayush Tiwari wrote: > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > struct landlock_object and update the related dependencies. This says what you do, but not why you want to do any of this. Why is this change needed? What benifit does it bring? And why did you cc: the staging mailing list? thanks, greg k-h
On Thu, Mar 28, 2024 at 04:55:57AM +0530, Ayush Tiwari wrote: > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > struct landlock_object and update the related dependencies. > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> Is there some advantage to doing this? You need to re-write the commit message to give us some clue why you are doing this. regards, dan carpenter
Hello Paul Thanks a lot for the feedback. Apologies for the mistakes. Could you help me in some places so that I can correct the errors, like: On Wed, Mar 27, 2024 at 07:43:36PM -0400, Paul Moore wrote: > On Wed, Mar 27, 2024 at 7:26 PM Ayush Tiwari <ayushtiw0110@gmail.com> wrote: > > > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > struct landlock_object and update the related dependencies. > > > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> > > --- > > security/landlock/fs.c | 2 +- > > security/landlock/object.c | 14 ++++++++++++-- > > security/landlock/object.h | 4 ++++ > > security/landlock/setup.c | 2 ++ > > 4 files changed, 19 insertions(+), 3 deletions(-) > > Hi Ayush, > > Mickaël has the final say on Landlock patches, but I had a few > comments that I've included below ... > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > > index fc520a06f9af..227dd67dd902 100644 > > --- a/security/landlock/fs.c > > +++ b/security/landlock/fs.c > > @@ -124,7 +124,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode) > > if (unlikely(rcu_access_pointer(inode_sec->object))) { > > /* Someone else just created the object, bail out and retry. */ > > spin_unlock(&inode->i_lock); > > - kfree(new_object); > > + kmem_cache_free(landlock_object_cache, new_object); > > See my comment below, but you may want to wrap this in a Landlock > object API function. Sure. I will definitely implement this. > > > rcu_read_lock(); > > goto retry; > > diff --git a/security/landlock/object.c b/security/landlock/object.c > > index 1f50612f0185..df1354215617 100644 > > --- a/security/landlock/object.c > > +++ b/security/landlock/object.c > > @@ -17,6 +17,15 @@ > > > > #include "object.h" > > > > +struct kmem_cache *landlock_object_cache; > > + > > +void __init landlock_object_init(void) > > +{ > > + landlock_object_cache = kmem_cache_create( > > + "landlock_object_cache", sizeof(struct landlock_object), 0, > > + SLAB_PANIC, NULL); > > The comments in include/linux/slab.h suggest using the KMEM_CACHE() > macro, instead of kmem_cache_create(), as a best practice for creating > slab caches. > Sure. Apologies I didn't see that, I tried to implement it from scratch using the reference from linux memory management APIs. > > +} > > + > > struct landlock_object * > > landlock_create_object(const struct landlock_object_underops *const underops, > > void *const underobj) > > @@ -25,7 +34,8 @@ landlock_create_object(const struct landlock_object_underops *const underops, > > > > if (WARN_ON_ONCE(!underops || !underobj)) > > return ERR_PTR(-ENOENT); > > - new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT); > > + new_object = > > + kmem_cache_zalloc(landlock_object_cache, GFP_KERNEL_ACCOUNT); > > If the line is too long, you might want to consider splitting the > function parameters like this: > > new_object = kmem_cache_zalloc(landlock_object_cache, > GFP_KERNEL_ACCOUNT); > Sure. I didn't do as it was below the 100 columns limit, but will definitely implement it. > > if (!new_object) > > return ERR_PTR(-ENOMEM); > > refcount_set(&new_object->usage, 1); > > @@ -62,6 +72,6 @@ void landlock_put_object(struct landlock_object *const object) > > * @object->underobj to @object (if it still exists). > > */ > > object->underops->release(object); > > - kfree_rcu(object, rcu_free); > > + kmem_cache_free(landlock_object_cache, object); > > } > > } > > diff --git a/security/landlock/object.h b/security/landlock/object.h > > index 5f28c35e8aa8..8ba1af3ddc2e 100644 > > --- a/security/landlock/object.h > > +++ b/security/landlock/object.h > > @@ -13,6 +13,10 @@ > > #include <linux/refcount.h> > > #include <linux/spinlock.h> > > > > +extern struct kmem_cache *landlock_object_cache; > > This really is a decision for Mickaël, but you may want to make > @landlock_object_cache private to object.c and create functions to > manage it as needed, e.g. put/free operations. > Okay. I didn't make it private as I was using it in fs.c to use kmem_cache_free, but if this is supposed to be private, I can modify the approach and expose it via some function, not directly exposing landlock_object_cache. > > +void __init landlock_object_init(void); > > + > > struct landlock_object; > > > > /** > > diff --git a/security/landlock/setup.c b/security/landlock/setup.c > > index f6dd33143b7f..a5fca4582ee1 100644 > > -- > paul-moore.com I will make all the changes you mentioned, and as you said, I will wait for Mickael's say.
On Thu, Mar 28, 2024 at 07:08:04AM +0100, Greg KH wrote: > On Thu, Mar 28, 2024 at 04:55:57AM +0530, Ayush Tiwari wrote: > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > struct landlock_object and update the related dependencies. > > This says what you do, but not why you want to do any of this. Why is > this change needed? What benifit does it bring? > > And why did you cc: the staging mailing list? > > thanks, > > greg k-h Hello Greg, Apologies for the errors. I will resend the patch with reason also for the changes I made. Earlier I didn't mention as I thought this patch is regarding an Issue from the Issues List. But will keep this in mind from now. Also, apologies for CC the staging mailing lists. I won't include it from next time if it's not needed. Thanks.
The subject should start with "landlock: Use" instead of "LANDLOCK: use" On Thu, Mar 28, 2024 at 01:23:17PM +0530, Ayush Tiwari wrote: > Hello Paul > Thanks a lot for the feedback. Apologies for the mistakes. Could you > help me in some places so that I can correct the errors, like: > On Wed, Mar 27, 2024 at 07:43:36PM -0400, Paul Moore wrote: > > On Wed, Mar 27, 2024 at 7:26 PM Ayush Tiwari <ayushtiw0110@gmail.com> wrote: > > > > > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > > struct landlock_object and update the related dependencies. > > > > > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> > > > --- > > > security/landlock/fs.c | 2 +- > > > security/landlock/object.c | 14 ++++++++++++-- > > > security/landlock/object.h | 4 ++++ > > > security/landlock/setup.c | 2 ++ > > > 4 files changed, 19 insertions(+), 3 deletions(-) > > > > Hi Ayush, > > > > Mickaël has the final say on Landlock patches, but I had a few > > comments that I've included below ... > > > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > > > index fc520a06f9af..227dd67dd902 100644 > > > --- a/security/landlock/fs.c > > > +++ b/security/landlock/fs.c > > > @@ -124,7 +124,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode) > > > if (unlikely(rcu_access_pointer(inode_sec->object))) { > > > /* Someone else just created the object, bail out and retry. */ > > > spin_unlock(&inode->i_lock); > > > - kfree(new_object); > > > + kmem_cache_free(landlock_object_cache, new_object); > > > > See my comment below, but you may want to wrap this in a Landlock > > object API function. > Sure. I will definitely implement this. > > > > > rcu_read_lock(); > > > goto retry; > > > diff --git a/security/landlock/object.c b/security/landlock/object.c > > > index 1f50612f0185..df1354215617 100644 > > > --- a/security/landlock/object.c > > > +++ b/security/landlock/object.c > > > @@ -17,6 +17,15 @@ > > > > > > #include "object.h" > > > > > > +struct kmem_cache *landlock_object_cache; > > > + > > > +void __init landlock_object_init(void) > > > +{ > > > + landlock_object_cache = kmem_cache_create( > > > + "landlock_object_cache", sizeof(struct landlock_object), 0, No need for the "_cache" name suffix. > > > + SLAB_PANIC, NULL); > > > > The comments in include/linux/slab.h suggest using the KMEM_CACHE() > > macro, instead of kmem_cache_create(), as a best practice for creating > > slab caches. > > > Sure. Apologies I didn't see that, I tried to implement it from scratch > using the reference from linux memory management APIs. > > > +} > > > + > > > struct landlock_object * > > > landlock_create_object(const struct landlock_object_underops *const underops, > > > void *const underobj) > > > @@ -25,7 +34,8 @@ landlock_create_object(const struct landlock_object_underops *const underops, > > > > > > if (WARN_ON_ONCE(!underops || !underobj)) > > > return ERR_PTR(-ENOENT); > > > - new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT); > > > + new_object = > > > + kmem_cache_zalloc(landlock_object_cache, GFP_KERNEL_ACCOUNT); > > > > If the line is too long, you might want to consider splitting the > > function parameters like this: > > > > new_object = kmem_cache_zalloc(landlock_object_cache, > > GFP_KERNEL_ACCOUNT); > > > > Sure. I didn't do as it was below the 100 columns limit, but will > definitely implement it. Please just use clang-format. > > > if (!new_object) > > > return ERR_PTR(-ENOMEM); > > > refcount_set(&new_object->usage, 1); > > > @@ -62,6 +72,6 @@ void landlock_put_object(struct landlock_object *const object) > > > * @object->underobj to @object (if it still exists). > > > */ > > > object->underops->release(object); > > > - kfree_rcu(object, rcu_free); Is it safe? According to commit ae65a5211d90 ("mm/slab: document kfree() as allowed for kmem_cache_alloc() objects"), no change should be needed (and it must not be backported to kernels older than 6.4 with CONFIG_SLOB). This way we can avoid exporting landlock_object_cache. Please add a note about this commit and the related warning in the commit message. > > > + kmem_cache_free(landlock_object_cache, object); > > > } > > > } > > > diff --git a/security/landlock/object.h b/security/landlock/object.h > > > index 5f28c35e8aa8..8ba1af3ddc2e 100644 > > > --- a/security/landlock/object.h > > > +++ b/security/landlock/object.h > > > @@ -13,6 +13,10 @@ > > > #include <linux/refcount.h> > > > #include <linux/spinlock.h> > > > > > > +extern struct kmem_cache *landlock_object_cache; > > > > This really is a decision for Mickaël, but you may want to make > > @landlock_object_cache private to object.c and create functions to > > manage it as needed, e.g. put/free operations. > > > Okay. I didn't make it private as I was using it in fs.c to use > kmem_cache_free, but if this is supposed to be private, I can modify the > approach and expose it via some function, not directly exposing > landlock_object_cache. Yes, that would be better. > > > +void __init landlock_object_init(void); > > > + > > > struct landlock_object; > > > > > > /** > > > diff --git a/security/landlock/setup.c b/security/landlock/setup.c > > > index f6dd33143b7f..a5fca4582ee1 100644 > > > > -- > > paul-moore.com > I will make all the changes you mentioned, and as you said, I will > wait for Mickael's say. Agree with Paul and Greg unless commented otherwise. Thanks
On Thu, Mar 28, 2024 at 09:39:14AM +0300, Dan Carpenter wrote: > On Thu, Mar 28, 2024 at 04:55:57AM +0530, Ayush Tiwari wrote: > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > struct landlock_object and update the related dependencies. > > > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> > > Is there some advantage to doing this? You need to re-write the commit > message to give us some clue why you are doing this. > > regards, > dan carpenter > Hello Apologies for the errors. I am working on a newer version, instilling all the corrections. Thanks for the feedback.
On Thu, Mar 28, 2024 at 03:45:12PM +0100, Mickaël Salaün wrote: > The subject should start with "landlock: Use" instead of "LANDLOCK: use" > > On Thu, Mar 28, 2024 at 01:23:17PM +0530, Ayush Tiwari wrote: > > Hello Paul > > Thanks a lot for the feedback. Apologies for the mistakes. Could you > > help me in some places so that I can correct the errors, like: > > On Wed, Mar 27, 2024 at 07:43:36PM -0400, Paul Moore wrote: > > > On Wed, Mar 27, 2024 at 7:26 PM Ayush Tiwari <ayushtiw0110@gmail.com> wrote: > > > > > > > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > > > struct landlock_object and update the related dependencies. > > > > > > > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> > > > > --- > > > > security/landlock/fs.c | 2 +- > > > > security/landlock/object.c | 14 ++++++++++++-- > > > > security/landlock/object.h | 4 ++++ > > > > security/landlock/setup.c | 2 ++ > > > > 4 files changed, 19 insertions(+), 3 deletions(-) > > > > > > Hi Ayush, > > > > > > Mickaël has the final say on Landlock patches, but I had a few > > > comments that I've included below ... > > > > > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > > > > index fc520a06f9af..227dd67dd902 100644 > > > > --- a/security/landlock/fs.c > > > > +++ b/security/landlock/fs.c > > > > @@ -124,7 +124,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode) > > > > if (unlikely(rcu_access_pointer(inode_sec->object))) { > > > > /* Someone else just created the object, bail out and retry. */ > > > > spin_unlock(&inode->i_lock); > > > > - kfree(new_object); > > > > + kmem_cache_free(landlock_object_cache, new_object); > > > > > > See my comment below, but you may want to wrap this in a Landlock > > > object API function. > > Sure. I will definitely implement this. > > > > > > > rcu_read_lock(); > > > > goto retry; > > > > diff --git a/security/landlock/object.c b/security/landlock/object.c > > > > index 1f50612f0185..df1354215617 100644 > > > > --- a/security/landlock/object.c > > > > +++ b/security/landlock/object.c > > > > @@ -17,6 +17,15 @@ > > > > > > > > #include "object.h" > > > > > > > > +struct kmem_cache *landlock_object_cache; > > > > + > > > > +void __init landlock_object_init(void) > > > > +{ > > > > + landlock_object_cache = kmem_cache_create( > > > > + "landlock_object_cache", sizeof(struct landlock_object), 0, > > No need for the "_cache" name suffix. > > > > > + SLAB_PANIC, NULL); > > > > > > The comments in include/linux/slab.h suggest using the KMEM_CACHE() > > > macro, instead of kmem_cache_create(), as a best practice for creating > > > slab caches. > > > Hello mentors I am facing some problem regarding replacing kzalloc with kmem_cache_zalloc when using KMEM macro from include/linux/slab.h as for kmem_cache_zalloc I will be needing a cache pointer to cache but KMEM macro doesn't provide any macro. So is there any way to do this or should I not use macro? > > Sure. Apologies I didn't see that, I tried to implement it from scratch > > using the reference from linux memory management APIs. > > > > +} > > > > + > > > > struct landlock_object * > > > > landlock_create_object(const struct landlock_object_underops *const underops, > > > > void *const underobj) > > > > @@ -25,7 +34,8 @@ landlock_create_object(const struct landlock_object_underops *const underops, > > > > > > > > if (WARN_ON_ONCE(!underops || !underobj)) > > > > return ERR_PTR(-ENOENT); > > > > - new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT); > > > > + new_object = > > > > + kmem_cache_zalloc(landlock_object_cache, GFP_KERNEL_ACCOUNT); > > > > > > If the line is too long, you might want to consider splitting the > > > function parameters like this: > > > > > > new_object = kmem_cache_zalloc(landlock_object_cache, > > > GFP_KERNEL_ACCOUNT); > > > > > > > Sure. I didn't do as it was below the 100 columns limit, but will > > definitely implement it. > > Please just use clang-format. > > > > > if (!new_object) > > > > return ERR_PTR(-ENOMEM); > > > > refcount_set(&new_object->usage, 1); > > > > @@ -62,6 +72,6 @@ void landlock_put_object(struct landlock_object *const object) > > > > * @object->underobj to @object (if it still exists). > > > > */ > > > > object->underops->release(object); > > > > - kfree_rcu(object, rcu_free); > > Is it safe? > > According to commit ae65a5211d90 ("mm/slab: document kfree() as allowed > for kmem_cache_alloc() objects"), no change should be needed (and it > must not be backported to kernels older than 6.4 with CONFIG_SLOB). This > way we can avoid exporting landlock_object_cache. Please add a note > about this commit and the related warning in the commit message. > > > > > + kmem_cache_free(landlock_object_cache, object); > > > > } > > > > } > > > > diff --git a/security/landlock/object.h b/security/landlock/object.h > > > > index 5f28c35e8aa8..8ba1af3ddc2e 100644 > > > > --- a/security/landlock/object.h > > > > +++ b/security/landlock/object.h > > > > @@ -13,6 +13,10 @@ > > > > #include <linux/refcount.h> > > > > #include <linux/spinlock.h> > > > > > > > > +extern struct kmem_cache *landlock_object_cache; > > > > > > This really is a decision for Mickaël, but you may want to make > > > @landlock_object_cache private to object.c and create functions to > > > manage it as needed, e.g. put/free operations. > > > > > Okay. I didn't make it private as I was using it in fs.c to use > > kmem_cache_free, but if this is supposed to be private, I can modify the > > approach and expose it via some function, not directly exposing > > landlock_object_cache. > > Yes, that would be better. > > > > > +void __init landlock_object_init(void); > > > > + > > > > struct landlock_object; > > > > > > > > /** > > > > diff --git a/security/landlock/setup.c b/security/landlock/setup.c > > > > index f6dd33143b7f..a5fca4582ee1 100644 > > > > > > -- > > > paul-moore.com > > I will make all the changes you mentioned, and as you said, I will > > wait for Mickael's say. > > Agree with Paul and Greg unless commented otherwise. Thanks
On Thu, Mar 28, 2024 at 03:45:12PM +0100, Mickaël Salaün wrote: > The subject should start with "landlock: Use" instead of "LANDLOCK: use" > > On Thu, Mar 28, 2024 at 01:23:17PM +0530, Ayush Tiwari wrote: > > Hello Paul > > Thanks a lot for the feedback. Apologies for the mistakes. Could you > > help me in some places so that I can correct the errors, like: > > On Wed, Mar 27, 2024 at 07:43:36PM -0400, Paul Moore wrote: > > > On Wed, Mar 27, 2024 at 7:26 PM Ayush Tiwari <ayushtiw0110@gmail.com> wrote: > > > > > > > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > > > struct landlock_object and update the related dependencies. > > > > > > > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> > > > > --- > > > > security/landlock/fs.c | 2 +- > > > > security/landlock/object.c | 14 ++++++++++++-- > > > > security/landlock/object.h | 4 ++++ > > > > security/landlock/setup.c | 2 ++ > > > > 4 files changed, 19 insertions(+), 3 deletions(-) > > > > > > Hi Ayush, > > > > > > Mickaël has the final say on Landlock patches, but I had a few > > > comments that I've included below ... > > > > > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > > > > index fc520a06f9af..227dd67dd902 100644 > > > > --- a/security/landlock/fs.c > > > > +++ b/security/landlock/fs.c > > > > @@ -124,7 +124,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode) > > > > if (unlikely(rcu_access_pointer(inode_sec->object))) { > > > > /* Someone else just created the object, bail out and retry. */ > > > > spin_unlock(&inode->i_lock); > > > > - kfree(new_object); > > > > + kmem_cache_free(landlock_object_cache, new_object); > > > > > > See my comment below, but you may want to wrap this in a Landlock > > > object API function. > > Sure. I will definitely implement this. > > > > > > > rcu_read_lock(); > > > > goto retry; > > > > diff --git a/security/landlock/object.c b/security/landlock/object.c > > > > index 1f50612f0185..df1354215617 100644 > > > > --- a/security/landlock/object.c > > > > +++ b/security/landlock/object.c > > > > @@ -17,6 +17,15 @@ > > > > > > > > #include "object.h" > > > > > > > > +struct kmem_cache *landlock_object_cache; > > > > + > > > > +void __init landlock_object_init(void) > > > > +{ > > > > + landlock_object_cache = kmem_cache_create( > > > > + "landlock_object_cache", sizeof(struct landlock_object), 0, > > No need for the "_cache" name suffix. > > > > > + SLAB_PANIC, NULL); > > > > > > The comments in include/linux/slab.h suggest using the KMEM_CACHE() > > > macro, instead of kmem_cache_create(), as a best practice for creating > > > slab caches. > > > Hello mentors I was trying to work on the above suggestion and I am facing some problem regarding replacing kzalloc with kmem_cache_zalloc calls when using KMEM macro from include/linux/slab.h because for kmem_cache_zalloc I will be needing a cache pointer, but KMEM macro doesn't return any such pointer. So is there any way to do this using macro or do i have to avoid using that macro for this case and use all methods regarding kmem as defined in the linux memory management API doc? > Agree with Paul and Greg unless commented otherwise. Thanks
diff --git a/security/landlock/fs.c b/security/landlock/fs.c index fc520a06f9af..227dd67dd902 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -124,7 +124,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode) if (unlikely(rcu_access_pointer(inode_sec->object))) { /* Someone else just created the object, bail out and retry. */ spin_unlock(&inode->i_lock); - kfree(new_object); + kmem_cache_free(landlock_object_cache, new_object); rcu_read_lock(); goto retry; diff --git a/security/landlock/object.c b/security/landlock/object.c index 1f50612f0185..df1354215617 100644 --- a/security/landlock/object.c +++ b/security/landlock/object.c @@ -17,6 +17,15 @@ #include "object.h" +struct kmem_cache *landlock_object_cache; + +void __init landlock_object_init(void) +{ + landlock_object_cache = kmem_cache_create( + "landlock_object_cache", sizeof(struct landlock_object), 0, + SLAB_PANIC, NULL); +} + struct landlock_object * landlock_create_object(const struct landlock_object_underops *const underops, void *const underobj) @@ -25,7 +34,8 @@ landlock_create_object(const struct landlock_object_underops *const underops, if (WARN_ON_ONCE(!underops || !underobj)) return ERR_PTR(-ENOENT); - new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT); + new_object = + kmem_cache_zalloc(landlock_object_cache, GFP_KERNEL_ACCOUNT); if (!new_object) return ERR_PTR(-ENOMEM); refcount_set(&new_object->usage, 1); @@ -62,6 +72,6 @@ void landlock_put_object(struct landlock_object *const object) * @object->underobj to @object (if it still exists). */ object->underops->release(object); - kfree_rcu(object, rcu_free); + kmem_cache_free(landlock_object_cache, object); } } diff --git a/security/landlock/object.h b/security/landlock/object.h index 5f28c35e8aa8..8ba1af3ddc2e 100644 --- a/security/landlock/object.h +++ b/security/landlock/object.h @@ -13,6 +13,10 @@ #include <linux/refcount.h> #include <linux/spinlock.h> +extern struct kmem_cache *landlock_object_cache; + +void __init landlock_object_init(void); + struct landlock_object; /** diff --git a/security/landlock/setup.c b/security/landlock/setup.c index f6dd33143b7f..a5fca4582ee1 100644 --- a/security/landlock/setup.c +++ b/security/landlock/setup.c @@ -16,6 +16,7 @@ #include "net.h" #include "ptrace.h" #include "setup.h" +#include "object.h" bool landlock_initialized __ro_after_init = false; @@ -33,6 +34,7 @@ const struct lsm_id landlock_lsmid = { static int __init landlock_init(void) { + landlock_object_init(); landlock_add_cred_hooks(); landlock_add_ptrace_hooks(); landlock_add_fs_hooks();
Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for struct landlock_object and update the related dependencies. Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> --- security/landlock/fs.c | 2 +- security/landlock/object.c | 14 ++++++++++++-- security/landlock/object.h | 4 ++++ security/landlock/setup.c | 2 ++ 4 files changed, 19 insertions(+), 3 deletions(-)