All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] selinux: various sparse fixes
@ 2022-01-27 18:21 Paul Moore
  2022-01-27 19:02 ` Ondrej Mosnacek
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2022-01-27 18:21 UTC (permalink / raw)
  To: selinux

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(&current->sighand->siglock);
+		spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
 		if (!fatal_signal_pending(current)) {
 			flush_sigqueue(&current->pending);
 			flush_sigqueue(&current->signal->shared_pending);
@@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
 			sigemptyset(&current->blocked);
 			recalc_sigpending();
 		}
-		spin_unlock_irq(&current->sighand->siglock);
+		spin_unlock_irq(unrcu_pointer(&current->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);


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] selinux: various sparse fixes
  2022-01-27 18:21 [PATCH v2] selinux: various sparse fixes Paul Moore
@ 2022-01-27 19:02 ` Ondrej Mosnacek
  2022-01-27 20:03   ` Paul Moore
  0 siblings, 1 reply; 6+ messages in thread
From: Ondrej Mosnacek @ 2022-01-27 19:02 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list

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(&current->sighand->siglock);
> +               spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
>                 if (!fatal_signal_pending(current)) {
>                         flush_sigqueue(&current->pending);
>                         flush_sigqueue(&current->signal->shared_pending);
> @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
>                         sigemptyset(&current->blocked);
>                         recalc_sigpending();
>                 }
> -               spin_unlock_irq(&current->sighand->siglock);
> +               spin_unlock_irq(unrcu_pointer(&current->sighand->siglock));

Shouldn't this be:

spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock);

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] selinux: various sparse fixes
  2022-01-27 19:02 ` Ondrej Mosnacek
@ 2022-01-27 20:03   ` Paul Moore
  2022-01-28 14:34     ` Ondrej Mosnacek
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2022-01-27 20:03 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: SElinux list

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(&current->sighand->siglock);
> > +               spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
> >                 if (!fatal_signal_pending(current)) {
> >                         flush_sigqueue(&current->pending);
> >                         flush_sigqueue(&current->signal->shared_pending);
> > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> >                         sigemptyset(&current->blocked);
> >                         recalc_sigpending();
> >                 }
> > -               spin_unlock_irq(&current->sighand->siglock);
> > +               spin_unlock_irq(unrcu_pointer(&current->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.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] selinux: various sparse fixes
  2022-01-27 20:03   ` Paul Moore
@ 2022-01-28 14:34     ` Ondrej Mosnacek
  2022-01-28 18:13       ` Paul Moore
  0 siblings, 1 reply; 6+ messages in thread
From: Ondrej Mosnacek @ 2022-01-28 14:34 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list

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(&current->sighand->siglock);
> > > +               spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
> > >                 if (!fatal_signal_pending(current)) {
> > >                         flush_sigqueue(&current->pending);
> > >                         flush_sigqueue(&current->signal->shared_pending);
> > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > >                         sigemptyset(&current->blocked);
> > >                         recalc_sigpending();
> > >                 }
> > > -               spin_unlock_irq(&current->sighand->siglock);
> > > +               spin_unlock_irq(unrcu_pointer(&current->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.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] selinux: various sparse fixes
  2022-01-28 14:34     ` Ondrej Mosnacek
@ 2022-01-28 18:13       ` Paul Moore
  2022-02-02  0:17         ` Paul Moore
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2022-01-28 18:13 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: SElinux list

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(&current->sighand->siglock);
> > > > +               spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
> > > >                 if (!fatal_signal_pending(current)) {
> > > >                         flush_sigqueue(&current->pending);
> > > >                         flush_sigqueue(&current->signal->shared_pending);
> > > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > > >                         sigemptyset(&current->blocked);
> > > >                         recalc_sigpending();
> > > >                 }
> > > > -               spin_unlock_irq(&current->sighand->siglock);
> > > > +               spin_unlock_irq(unrcu_pointer(&current->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.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] selinux: various sparse fixes
  2022-01-28 18:13       ` Paul Moore
@ 2022-02-02  0:17         ` Paul Moore
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Moore @ 2022-02-02  0:17 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: SElinux list

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(&current->sighand->siglock);
> > > > > +               spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
> > > > >                 if (!fatal_signal_pending(current)) {
> > > > >                         flush_sigqueue(&current->pending);
> > > > >                         flush_sigqueue(&current->signal->shared_pending);
> > > > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > > > >                         sigemptyset(&current->blocked);
> > > > >                         recalc_sigpending();
> > > > >                 }
> > > > > -               spin_unlock_irq(&current->sighand->siglock);
> > > > > +               spin_unlock_irq(unrcu_pointer(&current->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.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-02  0:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27 18:21 [PATCH v2] selinux: various sparse fixes Paul Moore
2022-01-27 19:02 ` Ondrej Mosnacek
2022-01-27 20:03   ` Paul Moore
2022-01-28 14:34     ` Ondrej Mosnacek
2022-01-28 18:13       ` Paul Moore
2022-02-02  0:17         ` Paul Moore

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.