Messages in this thread | | | From | Andrei Vagin <> | Date | Tue, 30 Aug 2022 09:07:56 -0700 | Subject | Re: [PATCH 1/4] seccomp: don't use semaphore and wait_queue together |
| |
On Tue, Aug 30, 2022 at 2:57 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > +static bool notify_wakeup(struct seccomp_filter *filter) > > +{ > > + bool ret; > > + > > + rcu_read_lock(); > > + ret = atomic_add_unless(&filter->notif->requests, -1, 0); > > Can you please spell out why the change to wait_event_interruptible() > below that calls notify_wakeup() makes it necessary to rcu protect > ->notif?
This is my mistake. rcu is used here when I tried to implement notify_wakeup without introducing notif->request. The idea was to enumerate all elements of notif->notifications. Now, it doesn't matter. In this context, filter->notif can be dereferenced without any additional locks. Thanks for catching this.
> > Given that you're using rcu_read_lock() here and the > WRITE_ONCE(fitler->notify, NULL) + kfree_rcu() sequence in > seccomp_notify_free() it seems to imply that filter->notif could be NULL > here and so would need a non-NULL check on ->notif before incrementing? > > > + rcu_read_unlock(); > > + > > + return ret; > > +} > > static long seccomp_notify_recv(struct seccomp_filter *filter, > void __user *buf) > @@ -1467,7 +1479,7 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, > > memset(&unotif, 0, sizeof(unotif)); > > - ret = down_interruptible(&filter->notif->request); > + ret = wait_event_interruptible(filter->wqh, notify_wakeup(filter)); > if (ret < 0) > return ret; >
Thanks, Andrei
| |