| Message ID | ZggZi/EFICvb4xTU@ayush-HP-Pavilion-Gaming-Laptop-15-ec0xxx (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] landlock: Use kmem for landlock_object | expand |
On Sat, Mar 30, 2024 at 07:24:19PM +0530, Ayush Tiwari wrote: > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > struct landlock_object and update the related dependencies to improve > memory allocation and deallocation performance. So it's faster? Great, what are the measurements? > This patch does not > change kfree() and kfree_rcu() calls because according to kernel commit > ae65a5211d90("mm/slab: document kfree() as allowed for > kmem_cache_alloc() objects"), starting from kernel 6.4 with > CONFIG_SLOB, kfree() is safe to use for such objects. There is no CONFIG_SLOB anymore so why mention it? > > Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> > --- > > Changes in v2: Used clang-format and corrected the removal of kfree_rcu. > Tried to use KMEM macro but due to lack of cache pointer in that macro, > had to explicitly define landlock_object_cache, as done in security.c. > > security/landlock/object.c | 12 +++++++++++- > security/landlock/object.h | 2 ++ > security/landlock/setup.c | 1 + > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/security/landlock/object.c b/security/landlock/object.c > index 1f50612f0185..cfc367725624 100644 > --- a/security/landlock/object.c > +++ b/security/landlock/object.c > @@ -17,6 +17,15 @@ > > #include "object.h" > > +static struct kmem_cache *landlock_object_cache; > + > +void __init landlock_object_cache_init(void) > +{ > + landlock_object_cache = kmem_cache_create( > + "landlock_object_cache", sizeof(struct landlock_object), 0, > + SLAB_PANIC, NULL); You really want SLAB_PANIC? Why? > +} > + > 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); Odd indentation, why? thanks, greg k-h
Hello Greg. Thanks for the feedback. On Sat, Mar 30, 2024 at 05:12:18PM +0100, Greg KH wrote: > On Sat, Mar 30, 2024 at 07:24:19PM +0530, Ayush Tiwari wrote: > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > struct landlock_object and update the related dependencies to improve > > memory allocation and deallocation performance. > > So it's faster? Great, what are the measurements? > Thank you for the feedback. Regarding the performance improvements, I realized I should have provided concrete measurements to support the claim. The intention behind switching to kmem_cache_zalloc() was to optimize memory management efficiency based on general principles. Could you suggest some way to get some measurements if possible? > > This patch does not > > change kfree() and kfree_rcu() calls because according to kernel commit > > ae65a5211d90("mm/slab: document kfree() as allowed for > > kmem_cache_alloc() objects"), starting from kernel 6.4 with > > CONFIG_SLOB, kfree() is safe to use for such objects. > > There is no CONFIG_SLOB anymore so why mention it? > About the mention of CONFIG_SLOB and commit ae65a5211d90, my aim was to highlight the kernel's documentation of using kfree() for objects allocated with kmem_cache_alloc(), ensuring this practice's safety across kernel versions post-6.4. I acknowledge that referencing CONFIG_SLOB was misleading due to its removal from the kernel configuration options. The focus should be on the broader acceptance and documentation of kfree() usage with kmem_cache allocated objects, independent of the specific allocator configuration. I will revise the patch description to clarify this point and remove any irrelevant references to CONFIG_SLOB. > > > > > +static struct kmem_cache *landlock_object_cache; > > + > > +void __init landlock_object_cache_init(void) > > +{ > > + landlock_object_cache = kmem_cache_create( > > + "landlock_object_cache", sizeof(struct landlock_object), 0, > > + SLAB_PANIC, NULL); > > You really want SLAB_PANIC? Why? > The SLAB_PANIC flag used in kmem_cache_create indicates that if the kernel is unable to create the cache, it should panic. The use of SLAB_PANIC in the creation of the landlock_object_cache is due to the critical nature of this cache for the Landlock LSM's operation. I found it to be a good choice to be used. Should I use some other altrnative? > > + > > 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); > > Odd indentation, why? > This indentation is due to formatting introduced by running clang-format. > thanks, > > greg k-h
On Sun, Mar 31, 2024 at 09:12:06PM +0530, Ayush Tiwari wrote: > Hello Greg. Thanks for the feedback. > On Sat, Mar 30, 2024 at 05:12:18PM +0100, Greg KH wrote: > > On Sat, Mar 30, 2024 at 07:24:19PM +0530, Ayush Tiwari wrote: > > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > > struct landlock_object and update the related dependencies to improve > > > memory allocation and deallocation performance. > > > > So it's faster? Great, what are the measurements? > > > Thank you for the feedback. Regarding the performance improvements, I > realized I should have provided concrete measurements to support the > claim. The intention behind switching to kmem_cache_zalloc() was to > optimize memory management efficiency based on general principles. > Could you suggest some way to get some measurements if possible? If you can not measure the difference, why make the change at all? Again, you need to prove the need for this change, so far I fail to see a reason why. > > > +static struct kmem_cache *landlock_object_cache; > > > + > > > +void __init landlock_object_cache_init(void) > > > +{ > > > + landlock_object_cache = kmem_cache_create( > > > + "landlock_object_cache", sizeof(struct landlock_object), 0, > > > + SLAB_PANIC, NULL); > > > > You really want SLAB_PANIC? Why? > > > The SLAB_PANIC flag used in kmem_cache_create indicates that if the > kernel is unable to create the cache, it should panic. The use of > SLAB_PANIC in the creation of the landlock_object_cache is due to the > critical nature of this cache for the Landlock LSM's operation. I > found it to be a good choice to be used. Should I use some other > altrnative? Is panicing really a good idea? Why can't you properly recover from allocation failures? > > > + > > > 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); > > > > Odd indentation, why? > > > This indentation is due to formatting introduced by running > clang-format. Why not keep it all on one line? thanks, greg k-h
On Sun, Mar 31, 2024 at 06:54:39PM +0200, Greg KH wrote: > On Sun, Mar 31, 2024 at 09:12:06PM +0530, Ayush Tiwari wrote: > > Hello Greg. Thanks for the feedback. > > On Sat, Mar 30, 2024 at 05:12:18PM +0100, Greg KH wrote: > > > On Sat, Mar 30, 2024 at 07:24:19PM +0530, Ayush Tiwari wrote: > > > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > > > struct landlock_object and update the related dependencies to improve > > > > memory allocation and deallocation performance. > > > > > > So it's faster? Great, what are the measurements? > > > > > Thank you for the feedback. Regarding the performance improvements, I > > realized I should have provided concrete measurements to support the > > claim. The intention behind switching to kmem_cache_zalloc() was to > > optimize memory management efficiency based on general principles. > > Could you suggest some way to get some measurements if possible? > > If you can not measure the difference, why make the change at all? Kindly refer to this issue: https://github.com/landlock-lsm/linux/issues/19 I have been assigned this issue hence I am focussing on making the changes that have been listed. > > Again, you need to prove the need for this change, so far I fail to see > a reason why. > > > > > +static struct kmem_cache *landlock_object_cache; > > > > + > > > > +void __init landlock_object_cache_init(void) > > > > +{ > > > > + landlock_object_cache = kmem_cache_create( > > > > + "landlock_object_cache", sizeof(struct landlock_object), 0, > > > > + SLAB_PANIC, NULL); > > > > > > You really want SLAB_PANIC? Why? > > > > > The SLAB_PANIC flag used in kmem_cache_create indicates that if the > > kernel is unable to create the cache, it should panic. The use of > > SLAB_PANIC in the creation of the landlock_object_cache is due to the > > critical nature of this cache for the Landlock LSM's operation. I > > found it to be a good choice to be used. Should I use some other > > altrnative? > > Is panicing really a good idea? Why can't you properly recover from > allocation failures? I am relying on SLAB_PANIC because of the reason I mentioned earlier, and also because it was used in lsm_file_cache that I was asked to look into as reference. I could try to recover from allocation failures but currently my focus is on working on the changes that are listed. I will definitely try to look into it once I am done with all changes. > > > > + > > > > 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); > > > > > > Odd indentation, why? > > > > > This indentation is due to formatting introduced by running > > clang-format. > > Why not keep it all on one line? > I kept it all in one line in v1, but Paul and Mickael asked me to use clang-format, hence it is this way. > thanks, > > greg k-h
On Sun, Mar 31, 2024 at 11:38:28PM +0530, Ayush Tiwari wrote: > On Sun, Mar 31, 2024 at 06:54:39PM +0200, Greg KH wrote: > > On Sun, Mar 31, 2024 at 09:12:06PM +0530, Ayush Tiwari wrote: > > > Hello Greg. Thanks for the feedback. > > > On Sat, Mar 30, 2024 at 05:12:18PM +0100, Greg KH wrote: > > > > On Sat, Mar 30, 2024 at 07:24:19PM +0530, Ayush Tiwari wrote: > > > > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for > > > > > struct landlock_object and update the related dependencies to improve > > > > > memory allocation and deallocation performance. > > > > > > > > So it's faster? Great, what are the measurements? > > > > > > > Thank you for the feedback. Regarding the performance improvements, I > > > realized I should have provided concrete measurements to support the > > > claim. The intention behind switching to kmem_cache_zalloc() was to > > > optimize memory management efficiency based on general principles. > > > Could you suggest some way to get some measurements if possible? > > > > If you can not measure the difference, why make the change at all? > > Kindly refer to this issue: https://github.com/landlock-lsm/linux/issues/19 > I have been assigned this issue hence I am focussing on making the > changes that have been listed. As Greg asked, it would be good know the performance impact of such change. This could be measured by creating a lot of related allocations and accessing them in non-sequential order (e.g. adding new rules, accessing a related inode while being sandboxed). I guess there will be a lot of noise (because of other subsystems) but it's worth a try. You should look at similar commits and their related threads to see what others did. > > > > Again, you need to prove the need for this change, so far I fail to see > > a reason why. > > > > > > > +static struct kmem_cache *landlock_object_cache; > > > > > + > > > > > +void __init landlock_object_cache_init(void) > > > > > +{ > > > > > + landlock_object_cache = kmem_cache_create( > > > > > + "landlock_object_cache", sizeof(struct landlock_object), 0, > > > > > + SLAB_PANIC, NULL); > > > > > > > > You really want SLAB_PANIC? Why? > > > > > > > The SLAB_PANIC flag used in kmem_cache_create indicates that if the > > > kernel is unable to create the cache, it should panic. The use of > > > SLAB_PANIC in the creation of the landlock_object_cache is due to the > > > critical nature of this cache for the Landlock LSM's operation. I > > > found it to be a good choice to be used. Should I use some other > > > altrnative? > > > > Is panicing really a good idea? Why can't you properly recover from > > allocation failures? > > I am relying on SLAB_PANIC because of the reason I mentioned earlier, > and also because it was used in lsm_file_cache that I was asked to look > into as reference. I could try to recover from allocation failures but > currently my focus is on working on the changes that are listed. I will > definitely try to look into it once I am done with all changes. Not being able to create this kmem cache would mean that Landlock would not be able to properly run, so we could print a warning and exit the Landlock init function. However, most calls to kmem_cache_create() are init calls, and most of them (especially in security/*) set SLAB_PANIC. I'm wondering why Landlock should do differently, if others should be fixed, and if the extra complexity of handling several kmem_cache_create() potential failure is worth it for init handlers? > > > > > > + > > > > > 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); > > > > > > > > Odd indentation, why? > > > > > > > This indentation is due to formatting introduced by running > > > clang-format. > > > > Why not keep it all on one line? > > > I kept it all in one line in v1, but Paul and Mickael asked me to use > clang-format, hence it is this way. Yes, it may looks weird but we format everything with clang-format to not waste time discussing about style. > > thanks, > > > > greg k-h >
diff --git a/security/landlock/object.c b/security/landlock/object.c index 1f50612f0185..cfc367725624 100644 --- a/security/landlock/object.c +++ b/security/landlock/object.c @@ -17,6 +17,15 @@ #include "object.h" +static struct kmem_cache *landlock_object_cache; + +void __init landlock_object_cache_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); diff --git a/security/landlock/object.h b/security/landlock/object.h index 5f28c35e8aa8..d9967ef16ec1 100644 --- a/security/landlock/object.h +++ b/security/landlock/object.h @@ -15,6 +15,8 @@ struct landlock_object; +void __init landlock_object_cache_init(void); + /** * struct landlock_object_underops - Operations on an underlying object */ diff --git a/security/landlock/setup.c b/security/landlock/setup.c index f6dd33143b7f..525820fc03ec 100644 --- a/security/landlock/setup.c +++ b/security/landlock/setup.c @@ -33,6 +33,7 @@ const struct lsm_id landlock_lsmid = { static int __init landlock_init(void) { + landlock_object_cache_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 to improve memory allocation and deallocation performance. This patch does not change kfree() and kfree_rcu() calls because according to kernel commit ae65a5211d90("mm/slab: document kfree() as allowed for kmem_cache_alloc() objects"), starting from kernel 6.4 with CONFIG_SLOB, kfree() is safe to use for such objects. Signed-off-by: Ayush Tiwari <ayushtiw0110@gmail.com> --- Changes in v2: Used clang-format and corrected the removal of kfree_rcu. Tried to use KMEM macro but due to lack of cache pointer in that macro, had to explicitly define landlock_object_cache, as done in security.c. security/landlock/object.c | 12 +++++++++++- security/landlock/object.h | 2 ++ security/landlock/setup.c | 1 + 3 files changed, 14 insertions(+), 1 deletion(-)