Re: [PATCH 1/5] seccomp: Add find_notification helper
On Sun, May 24, 2020 at 04:39:38PM -0700, Sargun Dhillon wrote: > This adds a helper which can iterate through a seccomp_filter to > find a notification matching an ID. It removes several replicated > chunks of code. > > Signed-off-by: Sargun Dhillon > Cc: Matt Denton > Cc: Kees Cook , > Cc: Jann Horn , > Cc: Robert Sesek , > Cc: Chris Palmer > Cc: Christian Brauner > Cc: Tycho Andersen > --- > kernel/seccomp.c | 38 +- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 55a6184f5990..f6ce94b7a167 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1021,10 +1021,25 @@ static int seccomp_notify_release(struct inode > *inode, struct file *file) > return 0; > } > > +/* must be called with notif_lock held */ > +static inline struct seccomp_knotif * > +find_notification(struct seccomp_filter *filter, u64 id) > +{ > + struct seccomp_knotif *cur; > + > + list_for_each_entry(cur, >notif->notifications, list) { > + if (cur->id == id) > + return cur; > + } > + > + return NULL; > +} > + > + > static long seccomp_notify_recv(struct seccomp_filter *filter, > void __user *buf) > { > - struct seccomp_knotif *knotif = NULL, *cur; > + struct seccomp_knotif *knotif, *cur; > struct seccomp_notif unotif; > ssize_t ret; > > @@ -1078,14 +1093,8 @@ static long seccomp_notify_recv(struct seccomp_filter > *filter, >* may have died when we released the lock, so we need to make >* sure it's still around. >*/ > - knotif = NULL; > mutex_lock(>notify_lock); > - list_for_each_entry(cur, >notif->notifications, list) { > - if (cur->id == unotif.id) { > - knotif = cur; > - break; > - } > - } > + knotif = find_notification(filter, unotif.id); > > if (knotif) { > knotif->state = SECCOMP_NOTIFY_INIT; > @@ -1150,7 +1159,7 @@ static long seccomp_notify_send(struct seccomp_filter > *filter, > static long seccomp_notify_id_valid(struct seccomp_filter *filter, > void __user *buf) > { > - struct seccomp_knotif *knotif = NULL; > + struct seccomp_knotif *knotif; > u64 id; > long ret; > > @@ -1162,15 +1171,10 @@ static long seccomp_notify_id_valid(struct > seccomp_filter *filter, > return ret; > > ret = -ENOENT; > - list_for_each_entry(knotif, >notif->notifications, list) { > - if (knotif->id == id) { > - if (knotif->state == SECCOMP_NOTIFY_SENT) > - ret = 0; > - goto out; > - } > - } > + knotif = find_notification(filter, id); > + if (knotif && knotif->state == SECCOMP_NOTIFY_SENT) > + ret = 0; Coul be a little nicer to have this be: if (knotif && knotif->state == SECCOMP_NOTIFY_SENT) ret = 0; else ret = -ENOENT; or, if you want to keep the assignment out of the lock: ret = -ENOENT; ret = mutex_lock_interruptible(>notify_lock); if (ret < 0) return ret; knotif = find_notification(filter, id); if (knotif && knotif->state == SECCOMP_NOTIFY_SENT) ret = 0; otherwise looks like a good cleanup to me.
Re: [PATCH 1/5] seccomp: Add find_notification helper
On Sun, May 24, 2020 at 04:39:38PM -0700, Sargun Dhillon wrote: > This adds a helper which can iterate through a seccomp_filter to > find a notification matching an ID. It removes several replicated > chunks of code. > > Signed-off-by: Sargun Dhillon > Cc: Matt Denton > Cc: Kees Cook , > Cc: Jann Horn , > Cc: Robert Sesek , > Cc: Chris Palmer > Cc: Christian Brauner > Cc: Tycho Andersen > --- > kernel/seccomp.c | 38 +- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 55a6184f5990..f6ce94b7a167 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1021,10 +1021,25 @@ static int seccomp_notify_release(struct inode > *inode, struct file *file) > return 0; > } > > +/* must be called with notif_lock held */ > +static inline struct seccomp_knotif * > +find_notification(struct seccomp_filter *filter, u64 id) > +{ > + struct seccomp_knotif *cur; > + > + list_for_each_entry(cur, >notif->notifications, list) { > + if (cur->id == id) > + return cur; > + } > + > + return NULL; > +} I think there's also an instance of this in _send() that we can change to use find_notification() as well. Tycho
[PATCH 1/5] seccomp: Add find_notification helper
This adds a helper which can iterate through a seccomp_filter to find a notification matching an ID. It removes several replicated chunks of code. Signed-off-by: Sargun Dhillon Cc: Matt Denton Cc: Kees Cook , Cc: Jann Horn , Cc: Robert Sesek , Cc: Chris Palmer Cc: Christian Brauner Cc: Tycho Andersen --- kernel/seccomp.c | 38 +- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 55a6184f5990..f6ce94b7a167 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1021,10 +1021,25 @@ static int seccomp_notify_release(struct inode *inode, struct file *file) return 0; } +/* must be called with notif_lock held */ +static inline struct seccomp_knotif * +find_notification(struct seccomp_filter *filter, u64 id) +{ + struct seccomp_knotif *cur; + + list_for_each_entry(cur, >notif->notifications, list) { + if (cur->id == id) + return cur; + } + + return NULL; +} + + static long seccomp_notify_recv(struct seccomp_filter *filter, void __user *buf) { - struct seccomp_knotif *knotif = NULL, *cur; + struct seccomp_knotif *knotif, *cur; struct seccomp_notif unotif; ssize_t ret; @@ -1078,14 +1093,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, * may have died when we released the lock, so we need to make * sure it's still around. */ - knotif = NULL; mutex_lock(>notify_lock); - list_for_each_entry(cur, >notif->notifications, list) { - if (cur->id == unotif.id) { - knotif = cur; - break; - } - } + knotif = find_notification(filter, unotif.id); if (knotif) { knotif->state = SECCOMP_NOTIFY_INIT; @@ -1150,7 +1159,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter, static long seccomp_notify_id_valid(struct seccomp_filter *filter, void __user *buf) { - struct seccomp_knotif *knotif = NULL; + struct seccomp_knotif *knotif; u64 id; long ret; @@ -1162,15 +1171,10 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter, return ret; ret = -ENOENT; - list_for_each_entry(knotif, >notif->notifications, list) { - if (knotif->id == id) { - if (knotif->state == SECCOMP_NOTIFY_SENT) - ret = 0; - goto out; - } - } + knotif = find_notification(filter, id); + if (knotif && knotif->state == SECCOMP_NOTIFY_SENT) + ret = 0; -out: mutex_unlock(>notify_lock); return ret; } -- 2.25.1