In keyctl_watch_key, use kfree_rcu() for freeing watch and wlist
as they support RCU and have an rcu_head in the struct definition.
Signed-off-by: Siddh Raman Pant <[email protected]>
---
security/keys/keyctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 96a92a645216..087fbc141cfd 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1832,9 +1832,9 @@ long keyctl_watch_key(key_serial_t id, int watch_queue_fd, int watch_id)
}
err_watch:
- kfree(watch);
+ kfree_rcu(watch, rcu);
err_wlist:
- kfree(wlist);
+ kfree_rcu(wlist, rcu);
err_wqueue:
put_watch_queue(wqueue);
err_key:
--
2.35.1
On Sat, Jul 23, 2022 at 07:20:35PM +0530, Siddh Raman Pant via Linux-kernel-mentees wrote:
> In keyctl_watch_key, use kfree_rcu() for freeing watch and wlist
> as they support RCU and have an rcu_head in the struct definition.
That does not explain why this change is needed. What problem does this
solve? Why use RCU if you don't have to? What functionality did you
just change in this commit and why?
And how was this tested?
thanks,
greg k-h
On Sat, 23 Jul 2022 19:35:16 +0530 Greg KH <[email protected]> wrote:
> That does not explain why this change is needed. What problem does this
> solve? Why use RCU if you don't have to? What functionality did you
> just change in this commit and why?
We can avoid a race condition wherein some process tries to access them while
they are being freed. For instance, the comment on `watch_queue_clear()` also
states that:
/*
* Remove all the watches that are contributory to a queue. This has the
* potential to race with removal of the watches by the destruction of the
* objects being watched or with the distribution of notifications.
*/
And an RCU read critical section is initiated in that function, so we should
use kfree_rcu() to not unintentionally free it while it is in the critical
section.
> And how was this tested?
It compiles locally for me, and I used syzbot on this along with testing the
other `watch_queue_clear` patch, which generated no errors.
Thanks,
Siddh
On Sat, 2022-07-23 at 20:05 +0530, Siddh Raman Pant wrote:
> On Sat, 23 Jul 2022 19:35:16 +0530 Greg KH <
> [email protected]> wrote:
> > That does not explain why this change is needed. What problem does
> > this solve? Why use RCU if you don't have to? What functionality
> > did you just change in this commit and why?
>
> We can avoid a race condition wherein some process tries to access
> them while they are being freed. For instance, the comment on
> `watch_queue_clear()` also states that:
> /*
> * Remove all the watches that are contributory to a
> queue. This has the
> * potential to race with removal of the watches by the
> destruction of the
> * objects being watched or with the distribution of
> notifications.
> */
> And an RCU read critical section is initiated in that function, so we
> should use kfree_rcu() to not unintentionally free it while it is in
> the critical section.
That doesn't apply in this case, does it? watch and wlist are locally
allocated and neither has been made externally visible if the error leg
is taken, so they should just be locally freed, which is what the code
was doing before this proposed patch.
James
On Sat, Jul 23, 2022 at 08:05:27PM +0530, Siddh Raman Pant wrote:
> On Sat, 23 Jul 2022 19:35:16 +0530 Greg KH <[email protected]> wrote:
> > That does not explain why this change is needed. What problem does this
> > solve? Why use RCU if you don't have to? What functionality did you
> > just change in this commit and why?
>
> We can avoid a race condition wherein some process tries to access them while
> they are being freed. For instance, the comment on `watch_queue_clear()` also
> states that:
> /*
> * Remove all the watches that are contributory to a queue. This has the
> * potential to race with removal of the watches by the destruction of the
> * objects being watched or with the distribution of notifications.
> */
> And an RCU read critical section is initiated in that function, so we should
> use kfree_rcu() to not unintentionally free it while it is in the critical
> section.
You need to explain all of this in a changelog text. Don't say what you
do, but say why you are doing it.
> > And how was this tested?
>
> It compiles locally for me, and I used syzbot on this along with testing the
> other `watch_queue_clear` patch, which generated no errors.
How does the watch queue stuff relate to this keyctl logic?
Again, be specific as to why you are doing things.
thanks,
greg k-h
On Sat, 23 Jul 2022 20:13:11 +0530 Greg KH <[email protected]> wrote:
> You need to explain all of this in a changelog text. Don't say what you
> do, but say why you are doing it.
Okay, I will keep this in mind next time.
> > > And how was this tested?
> >
> > It compiles locally for me, and I used syzbot on this along with testing the
> > other `watch_queue_clear` patch, which generated no errors.
>
> How does the watch queue stuff relate to this keyctl logic?
>
> Again, be specific as to why you are doing things.
It doesn't relate, I just wanted to say that syzbot didn't crash too (I had
this change in the same branch as that patch for testing, and syzbot compiled
it successfully).
Sorry for the confusion.
Though now as James has pointed out, this patch isn't needed.
Apologies,
Siddh
On Sat, 23 Jul 2022 20:20:49 +0530 James Bottomley <[email protected]> wrote:
> That doesn't apply in this case, does it? watch and wlist are locally
> allocated and neither has been made externally visible if the error leg
> is taken, so they should just be locally freed, which is what the code
> was doing before this proposed patch.
You are correct.
Sorry for this, I should have looked at it for a tad bit more.
Thanks,
Siddh
On Sat, Jul 23, 2022 at 07:20:35PM +0530, Siddh Raman Pant wrote:
> In keyctl_watch_key, use kfree_rcu() for freeing watch and wlist
> as they support RCU and have an rcu_head in the struct definition.
>
> Signed-off-by: Siddh Raman Pant <[email protected]>
Applies to any patch: the commit message should *clearly* describe
1. What is wrong in the current code *behaviour*.
2. Why does the code change save the day.
> ---
> security/keys/keyctl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 96a92a645216..087fbc141cfd 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1832,9 +1832,9 @@ long keyctl_watch_key(key_serial_t id, int watch_queue_fd, int watch_id)
> }
>
> err_watch:
> - kfree(watch);
> + kfree_rcu(watch, rcu);
> err_wlist:
> - kfree(wlist);
> + kfree_rcu(wlist, rcu);
> err_wqueue:
> put_watch_queue(wqueue);
> err_key:
> --
> 2.35.1
>
>
BR, Jarkko