| Message ID | 164330771809.139041.6643670399086580972.stgit@olly (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] selinux: various sparse fixes | expand |
On Thu, Jan 27, 2022 at 7:22 PM Paul Moore <paul@paul-moore.com> wrote: > When running the SELinux code through sparse, there are a handful of > warnings. This patch resolves some of these warnings caused by > "__rcu" mismatches. > > % make W=1 C=1 security/selinux/ > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/hooks.c | 6 +++--- > security/selinux/ibpkey.c | 2 +- > security/selinux/netnode.c | 5 +++-- > security/selinux/netport.c | 2 +- > 4 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 221e642025f5..0e857f86f5a7 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > if (rc) { > clear_itimer(); > > - spin_lock_irq(¤t->sighand->siglock); > + spin_lock_irq(unrcu_pointer(¤t->sighand->siglock)); > if (!fatal_signal_pending(current)) { > flush_sigqueue(¤t->pending); > flush_sigqueue(¤t->signal->shared_pending); > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > sigemptyset(¤t->blocked); > recalc_sigpending(); > } > - spin_unlock_irq(¤t->sighand->siglock); > + spin_unlock_irq(unrcu_pointer(¤t->sighand->siglock)); Shouldn't this be: spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock);
On Thu, Jan 27, 2022 at 2:03 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Jan 27, 2022 at 7:22 PM Paul Moore <paul@paul-moore.com> wrote: > > When running the SELinux code through sparse, there are a handful of > > warnings. This patch resolves some of these warnings caused by > > "__rcu" mismatches. > > > > % make W=1 C=1 security/selinux/ > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/selinux/hooks.c | 6 +++--- > > security/selinux/ibpkey.c | 2 +- > > security/selinux/netnode.c | 5 +++-- > > security/selinux/netport.c | 2 +- > > 4 files changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 221e642025f5..0e857f86f5a7 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > if (rc) { > > clear_itimer(); > > > > - spin_lock_irq(¤t->sighand->siglock); > > + spin_lock_irq(unrcu_pointer(¤t->sighand->siglock)); > > if (!fatal_signal_pending(current)) { > > flush_sigqueue(¤t->pending); > > flush_sigqueue(¤t->signal->shared_pending); > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > sigemptyset(¤t->blocked); > > recalc_sigpending(); > > } > > - spin_unlock_irq(¤t->sighand->siglock); > > + spin_unlock_irq(unrcu_pointer(¤t->sighand->siglock)); > > Shouldn't this be: > > spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock); Maybe. The __rcu space annotation is definitely on task_struct::sighand, but my (quick) look at unrcu_pointer() was that the the de-rcu'ification applies to all of the dereferencing that is passed as the macro argument. Because of that I decided to pass the entire dereferencing chain to the unrcu_pointer() macro just in case. If that way of thinking is incorrect please let me know, otherwise I would rather just leave it as it is in v2.
On Thu, Jan 27, 2022 at 9:04 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Jan 27, 2022 at 2:03 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Thu, Jan 27, 2022 at 7:22 PM Paul Moore <paul@paul-moore.com> wrote: > > > When running the SELinux code through sparse, there are a handful of > > > warnings. This patch resolves some of these warnings caused by > > > "__rcu" mismatches. > > > > > > % make W=1 C=1 security/selinux/ > > > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > --- > > > security/selinux/hooks.c | 6 +++--- > > > security/selinux/ibpkey.c | 2 +- > > > security/selinux/netnode.c | 5 +++-- > > > security/selinux/netport.c | 2 +- > > > 4 files changed, 8 insertions(+), 7 deletions(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 221e642025f5..0e857f86f5a7 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > > if (rc) { > > > clear_itimer(); > > > > > > - spin_lock_irq(¤t->sighand->siglock); > > > + spin_lock_irq(unrcu_pointer(¤t->sighand->siglock)); > > > if (!fatal_signal_pending(current)) { > > > flush_sigqueue(¤t->pending); > > > flush_sigqueue(¤t->signal->shared_pending); > > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > > sigemptyset(¤t->blocked); > > > recalc_sigpending(); > > > } > > > - spin_unlock_irq(¤t->sighand->siglock); > > > + spin_unlock_irq(unrcu_pointer(¤t->sighand->siglock)); > > > > Shouldn't this be: > > > > spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock); > > Maybe. > > The __rcu space annotation is definitely on task_struct::sighand, but > my (quick) look at unrcu_pointer() was that the the de-rcu'ification > applies to all of the dereferencing that is passed as the macro > argument. Because of that I decided to pass the entire dereferencing > chain to the unrcu_pointer() macro just in case. If that way of > thinking is incorrect please let me know, otherwise I would rather > just leave it as it is in v2. While it does work this way, too, it just doesn't feel right. IMHO it's more self-documenting to mark the exact pointer for which we are applying the RCU access exception. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Fri, Jan 28, 2022 at 9:35 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Jan 27, 2022 at 9:04 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Jan 27, 2022 at 2:03 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > On Thu, Jan 27, 2022 at 7:22 PM Paul Moore <paul@paul-moore.com> wrote: > > > > When running the SELinux code through sparse, there are a handful of > > > > warnings. This patch resolves some of these warnings caused by > > > > "__rcu" mismatches. > > > > > > > > % make W=1 C=1 security/selinux/ > > > > > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > --- > > > > security/selinux/hooks.c | 6 +++--- > > > > security/selinux/ibpkey.c | 2 +- > > > > security/selinux/netnode.c | 5 +++-- > > > > security/selinux/netport.c | 2 +- > > > > 4 files changed, 8 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index 221e642025f5..0e857f86f5a7 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > > > if (rc) { > > > > clear_itimer(); > > > > > > > > - spin_lock_irq(¤t->sighand->siglock); > > > > + spin_lock_irq(unrcu_pointer(¤t->sighand->siglock)); > > > > if (!fatal_signal_pending(current)) { > > > > flush_sigqueue(¤t->pending); > > > > flush_sigqueue(¤t->signal->shared_pending); > > > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > > > sigemptyset(¤t->blocked); > > > > recalc_sigpending(); > > > > } > > > > - spin_unlock_irq(¤t->sighand->siglock); > > > > + spin_unlock_irq(unrcu_pointer(¤t->sighand->siglock)); > > > > > > Shouldn't this be: > > > > > > spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock); > > > > Maybe. > > > > The __rcu space annotation is definitely on task_struct::sighand, but > > my (quick) look at unrcu_pointer() was that the the de-rcu'ification > > applies to all of the dereferencing that is passed as the macro > > argument. Because of that I decided to pass the entire dereferencing > > chain to the unrcu_pointer() macro just in case. If that way of > > thinking is incorrect please let me know, otherwise I would rather > > just leave it as it is in v2. > > While it does work this way, too, it just doesn't feel right. IMHO > it's more self-documenting to mark the exact pointer for which we are > applying the RCU access exception. FWIW, the documentation aspect here is the "__rcu" marking on the pointer in the task_struct, and possibly the sparse warnings. What we are doing here is basically a hack to tell sparse to be quiet, and since these core kernel variable annotations are likely to come and go independent of the SELinux code I'm not overly worried about minor self-documenting issues such as this (they are going to go out-of-date anyway). I'm more concerned with keeping a clean {sparse} build.
On Fri, Jan 28, 2022 at 1:13 PM Paul Moore <paul@paul-moore.com> wrote: > On Fri, Jan 28, 2022 at 9:35 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Jan 27, 2022 at 9:04 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Thu, Jan 27, 2022 at 2:03 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > > > On Thu, Jan 27, 2022 at 7:22 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > When running the SELinux code through sparse, there are a handful of > > > > > warnings. This patch resolves some of these warnings caused by > > > > > "__rcu" mismatches. > > > > > > > > > > % make W=1 C=1 security/selinux/ > > > > > > > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > > --- > > > > > security/selinux/hooks.c | 6 +++--- > > > > > security/selinux/ibpkey.c | 2 +- > > > > > security/selinux/netnode.c | 5 +++-- > > > > > security/selinux/netport.c | 2 +- > > > > > 4 files changed, 8 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > > index 221e642025f5..0e857f86f5a7 100644 > > > > > --- a/security/selinux/hooks.c > > > > > +++ b/security/selinux/hooks.c > > > > > @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > > > > if (rc) { > > > > > clear_itimer(); > > > > > > > > > > - spin_lock_irq(¤t->sighand->siglock); > > > > > + spin_lock_irq(unrcu_pointer(¤t->sighand->siglock)); > > > > > if (!fatal_signal_pending(current)) { > > > > > flush_sigqueue(¤t->pending); > > > > > flush_sigqueue(¤t->signal->shared_pending); > > > > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) > > > > > sigemptyset(¤t->blocked); > > > > > recalc_sigpending(); > > > > > } > > > > > - spin_unlock_irq(¤t->sighand->siglock); > > > > > + spin_unlock_irq(unrcu_pointer(¤t->sighand->siglock)); > > > > > > > > Shouldn't this be: > > > > > > > > spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock); > > > > > > Maybe. > > > > > > The __rcu space annotation is definitely on task_struct::sighand, but > > > my (quick) look at unrcu_pointer() was that the the de-rcu'ification > > > applies to all of the dereferencing that is passed as the macro > > > argument. Because of that I decided to pass the entire dereferencing > > > chain to the unrcu_pointer() macro just in case. If that way of > > > thinking is incorrect please let me know, otherwise I would rather > > > just leave it as it is in v2. > > > > While it does work this way, too, it just doesn't feel right. IMHO > > it's more self-documenting to mark the exact pointer for which we are > > applying the RCU access exception. > > FWIW, the documentation aspect here is the "__rcu" marking on the > pointer in the task_struct, and possibly the sparse warnings. What we > are doing here is basically a hack to tell sparse to be quiet, and > since these core kernel variable annotations are likely to come and go > independent of the SELinux code I'm not overly worried about minor > self-documenting issues such as this (they are going to go out-of-date > anyway). I'm more concerned with keeping a clean {sparse} build. Merged into selinux/next, and ultimately I changed my mind and decided to change the current::sighand lines as Ondrej suggested.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 221e642025f5..0e857f86f5a7 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) if (rc) { clear_itimer(); - spin_lock_irq(¤t->sighand->siglock); + spin_lock_irq(unrcu_pointer(¤t->sighand->siglock)); if (!fatal_signal_pending(current)) { flush_sigqueue(¤t->pending); flush_sigqueue(¤t->signal->shared_pending); @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm) sigemptyset(¤t->blocked); recalc_sigpending(); } - spin_unlock_irq(¤t->sighand->siglock); + spin_unlock_irq(unrcu_pointer(¤t->sighand->siglock)); } /* Wake up the parent if it is waiting so that it can recheck * wait permission to the new task SID. */ read_lock(&tasklist_lock); - __wake_up_parent(current, current->real_parent); + __wake_up_parent(current, unrcu_pointer(current->real_parent)); read_unlock(&tasklist_lock); } diff --git a/security/selinux/ibpkey.c b/security/selinux/ibpkey.c index 20b3b2243820..5839ca7bb9c7 100644 --- a/security/selinux/ibpkey.c +++ b/security/selinux/ibpkey.c @@ -104,7 +104,7 @@ static void sel_ib_pkey_insert(struct sel_ib_pkey *pkey) tail = list_entry( rcu_dereference_protected( - sel_ib_pkey_hash[idx].list.prev, + list_tail_rcu(&sel_ib_pkey_hash[idx].list), lockdep_is_held(&sel_ib_pkey_lock)), struct sel_ib_pkey, list); list_del_rcu(&tail->list); diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c index 889552db0d31..0ac7df9a9367 100644 --- a/security/selinux/netnode.c +++ b/security/selinux/netnode.c @@ -164,8 +164,9 @@ static void sel_netnode_insert(struct sel_netnode *node) if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT) { struct sel_netnode *tail; tail = list_entry( - rcu_dereference_protected(sel_netnode_hash[idx].list.prev, - lockdep_is_held(&sel_netnode_lock)), + rcu_dereference_protected( + list_tail_rcu(&sel_netnode_hash[idx].list), + lockdep_is_held(&sel_netnode_lock)), struct sel_netnode, list); list_del_rcu(&tail->list); kfree_rcu(tail, rcu); diff --git a/security/selinux/netport.c b/security/selinux/netport.c index 9ba09d11c0f5..8eec6347cf01 100644 --- a/security/selinux/netport.c +++ b/security/selinux/netport.c @@ -113,7 +113,7 @@ static void sel_netport_insert(struct sel_netport *port) struct sel_netport *tail; tail = list_entry( rcu_dereference_protected( - sel_netport_hash[idx].list.prev, + list_tail_rcu(&sel_netport_hash[idx].list), lockdep_is_held(&sel_netport_lock)), struct sel_netport, list); list_del_rcu(&tail->list);
When running the SELinux code through sparse, there are a handful of warnings. This patch resolves some of these warnings caused by "__rcu" mismatches. % make W=1 C=1 security/selinux/ Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/hooks.c | 6 +++--- security/selinux/ibpkey.c | 2 +- security/selinux/netnode.c | 5 +++-- security/selinux/netport.c | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-)