Alexander Kapshuk wrote on Thu, Jun 18, 2020:
> Address sparse nonderef rcu warnings:
> net/9p/client.c:790:17: warning: incorrect type in argument 1 (different address spaces)
> net/9p/client.c:790:17: expected struct spinlock [usertype] *lock
> net/9p/client.c:790:17: got struct spinlock [noderef] <asn:4> *
> net/9p/client.c:792:48: warning: incorrect type in argument 1 (different address spaces)
> net/9p/client.c:792:48: expected struct spinlock [usertype] *lock
> net/9p/client.c:792:48: got struct spinlock [noderef] <asn:4> *
> net/9p/client.c:872:17: warning: incorrect type in argument 1 (different address spaces)
> net/9p/client.c:872:17: expected struct spinlock [usertype] *lock
> net/9p/client.c:872:17: got struct spinlock [noderef] <asn:4> *
> net/9p/client.c:874:48: warning: incorrect type in argument 1 (different address spaces)
> net/9p/client.c:874:48: expected struct spinlock [usertype] *lock
> net/9p/client.c:874:48: got struct spinlock [noderef] <asn:4> *
>
> Signed-off-by: Alexander Kapshuk <[email protected]>
Thanks for this patch.
From what I can see, there are tons of other parts of the code doing the
same noderef access pattern to access current->sighand->siglock and I
don't see much doing that.
A couple of users justify this by saying SLAB_TYPESAFE_BY_RCU ensures
we'll always get a usable lock which won't be reinitialized however we
access it... It's a bit dubious we'll get the same lock than unlock to
me, so I agree to some change though.
After a second look I think we should use something like the following:
if (!lock_task_sighand(current, &flags))
warn & skip (or some error, we'd null deref if this happened currently);
recalc_sigpending();
unlock_task_sighand(current, &flags);
As you can see, the rcu_read_lock() isn't kept until the unlock so I'm
not sure it will be enough to please sparse, but I've convinced myself
current->sighand cannot change while we hold the lock and there just are
too many such patterns in the kernel.
Please let me know if I missed something or if there is an ongoing
effort to change how this works; I'll wait for a v2.
--
Dominique
On Thu, Jun 18, 2020 at 10:08 PM Dominique Martinet
<[email protected]> wrote:
>
> Alexander Kapshuk wrote on Thu, Jun 18, 2020:
> > Address sparse nonderef rcu warnings:
> > net/9p/client.c:790:17: warning: incorrect type in argument 1 (different address spaces)
> > net/9p/client.c:790:17: expected struct spinlock [usertype] *lock
> > net/9p/client.c:790:17: got struct spinlock [noderef] <asn:4> *
> > net/9p/client.c:792:48: warning: incorrect type in argument 1 (different address spaces)
> > net/9p/client.c:792:48: expected struct spinlock [usertype] *lock
> > net/9p/client.c:792:48: got struct spinlock [noderef] <asn:4> *
> > net/9p/client.c:872:17: warning: incorrect type in argument 1 (different address spaces)
> > net/9p/client.c:872:17: expected struct spinlock [usertype] *lock
> > net/9p/client.c:872:17: got struct spinlock [noderef] <asn:4> *
> > net/9p/client.c:874:48: warning: incorrect type in argument 1 (different address spaces)
> > net/9p/client.c:874:48: expected struct spinlock [usertype] *lock
> > net/9p/client.c:874:48: got struct spinlock [noderef] <asn:4> *
> >
> > Signed-off-by: Alexander Kapshuk <[email protected]>
>
> Thanks for this patch.
> From what I can see, there are tons of other parts of the code doing the
> same noderef access pattern to access current->sighand->siglock and I
> don't see much doing that.
> A couple of users justify this by saying SLAB_TYPESAFE_BY_RCU ensures
> we'll always get a usable lock which won't be reinitialized however we
> access it... It's a bit dubious we'll get the same lock than unlock to
> me, so I agree to some change though.
>
> After a second look I think we should use something like the following:
>
> if (!lock_task_sighand(current, &flags))
> warn & skip (or some error, we'd null deref if this happened currently);
> recalc_sigpending();
> unlock_task_sighand(current, &flags);
>
> As you can see, the rcu_read_lock() isn't kept until the unlock so I'm
> not sure it will be enough to please sparse, but I've convinced myself
> current->sighand cannot change while we hold the lock and there just are
> too many such patterns in the kernel.
>
> Please let me know if I missed something or if there is an ongoing
> effort to change how this works; I'll wait for a v2.
>
> --
> Dominique
Thanks for your prompt response.
I too made the same observation of the numerous patterns in the kernel
where current->sighand is accessed without being rcu_dereference()'d.
For this patch I used kernel/signal.c:1368,1398: __lock_task_sighand()
as an example.
I will give your suggestion a careful consideration and will get back
to you soon.
Thanks.
Use (un)lock_task_sighand instead of spin_lock_irqsave and
spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
suggested in the email referenced below.
Signed-off-by: Alexander Kapshuk <[email protected]>
Link: https://lore.kernel.org/lkml/20200618190807.GA20699@nautica/
---
net/9p/client.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/net/9p/client.c b/net/9p/client.c
index fc1f3635e5dd..15f16f2baa8f 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -787,9 +787,14 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
}
recalc_sigpending:
if (sigpending) {
- spin_lock_irqsave(¤t->sighand->siglock, flags);
+ if (!lock_task_sighand(current, &flags)) {
+ pr_warn("%s (%d): current->sighand==NULL in recalc_sigpending\n",
+ __func__, task_pid_nr(current));
+ err = -ESRCH;
+ goto reterr;
+ }
recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, flags);
+ unlock_task_sighand(current, &flags);
}
if (err < 0)
goto reterr;
@@ -869,9 +874,14 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
}
recalc_sigpending:
if (sigpending) {
- spin_lock_irqsave(¤t->sighand->siglock, flags);
+ if (!lock_task_sighand(current, &flags)) {
+ pr_warn("%s (%d): current->sighand==NULL in recalc_sigpending\n",
+ __func__, task_pid_nr(current));
+ err = -ESRCH;
+ goto reterr;
+ }
recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, flags);
+ unlock_task_sighand(current, &flags);
}
if (err < 0)
goto reterr;
--
2.27.0
Alexander Kapshuk wrote on Sat, Jun 20, 2020:
> Use (un)lock_task_sighand instead of spin_lock_irqsave and
> spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
> suggested in the email referenced below.
Thanks for v2! Patch itself looks good to me.
I always add another `Link:` tag to the last version of the patch at the
time of applying, so the message might be a bit confusing.
Feel free to keep the link to the previous discussion but I'd rather
just repeat a bit more of what we discussed (e.g. fix rcu not being
dereferenced cleanly by using the task helpers as suggested) rather than
just link to the thread
Sorry for nitpicking but I think commit messages are important and it's
better if they're understandable out of context, even if you give a link
for further details for curious readers, it helps being able to just
skim through git log.
Either way I'll include the patch in my test run today or tomorrow, had
promised it for a while...
Cheers,
--
Dominique
On Sun, Jun 21, 2020 at 11:45 AM Dominique Martinet
<[email protected]> wrote:
>
> Alexander Kapshuk wrote on Sat, Jun 20, 2020:
> > Use (un)lock_task_sighand instead of spin_lock_irqsave and
> > spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
> > suggested in the email referenced below.
>
> Thanks for v2! Patch itself looks good to me.
>
> I always add another `Link:` tag to the last version of the patch at the
> time of applying, so the message might be a bit confusing.
> Feel free to keep the link to the previous discussion but I'd rather
> just repeat a bit more of what we discussed (e.g. fix rcu not being
> dereferenced cleanly by using the task helpers as suggested) rather than
> just link to the thread
>
> Sorry for nitpicking but I think commit messages are important and it's
> better if they're understandable out of context, even if you give a link
> for further details for curious readers, it helps being able to just
> skim through git log.
>
>
> Either way I'll include the patch in my test run today or tomorrow, had
> promised it for a while...
>
> Cheers,
> --
> Dominique
Hi Dominique,
Thanks for your feedback.
Shall I simply resend the v2 patch with the commit message reworded as
you suggested, or should I make it a v3 patch instead?
One other thing I wanted to clarify is I got a message from kernel
test robot <[email protected]>,
https://lists.01.org/hyperkitty/list/[email protected]/thread/TMTLPYU6A522JH2VCN3PNZVAP6EE5MDF/,
saying that on parisc my patch resulted in __lock_task_sighand being
undefined during modpost'ing.
I've noticed similar messages about other people's patches on the
linux-kernel mailing list with the responses stating that the issue
was at the compilation site rather than with the patch itself.
As far as I understand, that is the case with my patch also. So no
further action on that is required of me, is it?
Thanks.
Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> Thanks for your feedback.
> Shall I simply resend the v2 patch with the commit message reworded as
> you suggested, or should I make it a v3 patch instead?
Yes please resend the same commit reworded. v2 or v3 doesn't matter
much, the [PATCH whatever] part of the mail isn't included in the commit
message; I don't receive so much patches to be fussy about that :)
> One other thing I wanted to clarify is I got a message from kernel
> test robot <[email protected]>,
> https://lists.01.org/hyperkitty/list/[email protected]/thread/TMTLPYU6A522JH2VCN3PNZVAP6EE5MDF/,
> saying that on parisc my patch resulted in __lock_task_sighand being
> undefined during modpost'ing.
> I've noticed similar messages about other people's patches on the
> linux-kernel mailing list with the responses stating that the issue
> was at the compilation site rather than with the patch itself.
> As far as I understand, that is the case with my patch also. So no
> further action on that is required of me, is it?
Thanks, I hadn't noticed this mail -- unfortunately I don't have
anything setup to test pa risc, but __lock_task_sighand is defined in
kernel/signal.c which is unconditionally compiled so shouldn't have
anything to do with arch-specific failures, so I will run into the same
problem when I test this.
From just looking at the code, it looks like a real problem to me -
__lock_task_sighand is never passed to EXPORT_SYMBOL so when building 9p
as a module we cannot use the function. Only exported symbols can be
called from code that can be built as modules.
That looks like an oversight to me more than something on purpose, but
it does mean I cannot take the patch right now -- we need to first get
the symbol exported before we can use it in 9p.
As things stand I'd rather have this patch wait one cycle for this than
revert to manipulating rcu directly like you did first -- if you're up
for it you can send a patch to get it exported first and I'll pick this
patch up next cycle, or I can take care of that too if you don't want to
bother.
Letting you tell me which you prefer,
--
Dominique
On Sun, Jun 21, 2020 at 1:57 PM Dominique Martinet
<[email protected]> wrote:
>
> Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> > Thanks for your feedback.
> > Shall I simply resend the v2 patch with the commit message reworded as
> > you suggested, or should I make it a v3 patch instead?
>
> Yes please resend the same commit reworded. v2 or v3 doesn't matter
> much, the [PATCH whatever] part of the mail isn't included in the commit
> message; I don't receive so much patches to be fussy about that :)
>
Understood. Thanks. :)
> > One other thing I wanted to clarify is I got a message from kernel
> > test robot <[email protected]>,
> > https://lists.01.org/hyperkitty/list/[email protected]/thread/TMTLPYU6A522JH2VCN3PNZVAP6EE5MDF/,
> > saying that on parisc my patch resulted in __lock_task_sighand being
> > undefined during modpost'ing.
> > I've noticed similar messages about other people's patches on the
> > linux-kernel mailing list with the responses stating that the issue
> > was at the compilation site rather than with the patch itself.
> > As far as I understand, that is the case with my patch also. So no
> > further action on that is required of me, is it?
>
> Thanks, I hadn't noticed this mail -- unfortunately I don't have
> anything setup to test pa risc, but __lock_task_sighand is defined in
> kernel/signal.c which is unconditionally compiled so shouldn't have
> anything to do with arch-specific failures, so I will run into the same
> problem when I test this.
>
> From just looking at the code, it looks like a real problem to me -
> __lock_task_sighand is never passed to EXPORT_SYMBOL so when building 9p
> as a module we cannot use the function. Only exported symbols can be
> called from code that can be built as modules.
>
> That looks like an oversight to me more than something on purpose, but
> it does mean I cannot take the patch right now -- we need to first get
> the symbol exported before we can use it in 9p.
>
>
> As things stand I'd rather have this patch wait one cycle for this than
> revert to manipulating rcu directly like you did first -- if you're up
> for it you can send a patch to get it exported first and I'll pick this
> patch up next cycle, or I can take care of that too if you don't want to
> bother.
>
> Letting you tell me which you prefer,
> --
> Dominique
I am willing to send in a patch to have the missing symbol exported as
well as resend my previous one with the commit message reworded.
Thanks.
Fix rcu not being dereferenced cleanly by using the task
helpers (un)lock_task_sighand instead of spin_lock_irqsave and
spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
suggested in the email referenced below.
Signed-off-by: Alexander Kapshuk <[email protected]>
Link: https://lore.kernel.org/lkml/20200618190807.GA20699@nautica/
---
net/9p/client.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/net/9p/client.c b/net/9p/client.c
index fc1f3635e5dd..15f16f2baa8f 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -787,9 +787,14 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
}
recalc_sigpending:
if (sigpending) {
- spin_lock_irqsave(¤t->sighand->siglock, flags);
+ if (!lock_task_sighand(current, &flags)) {
+ pr_warn("%s (%d): current->sighand==NULL in recalc_sigpending\n",
+ __func__, task_pid_nr(current));
+ err = -ESRCH;
+ goto reterr;
+ }
recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, flags);
+ unlock_task_sighand(current, &flags);
}
if (err < 0)
goto reterr;
@@ -869,9 +874,14 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
}
recalc_sigpending:
if (sigpending) {
- spin_lock_irqsave(¤t->sighand->siglock, flags);
+ if (!lock_task_sighand(current, &flags)) {
+ pr_warn("%s (%d): current->sighand==NULL in recalc_sigpending\n",
+ __func__, task_pid_nr(current));
+ err = -ESRCH;
+ goto reterr;
+ }
recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, flags);
+ unlock_task_sighand(current, &flags);
}
if (err < 0)
goto reterr;
--
2.27.0
Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> Fix rcu not being dereferenced cleanly by using the task
> helpers (un)lock_task_sighand instead of spin_lock_irqsave and
> spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
> suggested in the email referenced below.
Ack.
I'll take this once symbol issue is resolved ; thanks for your time.
--
Dominique
On Sun, Jun 21, 2020 at 4:56 PM Dominique Martinet
<[email protected]> wrote:
>
> Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> > Fix rcu not being dereferenced cleanly by using the task
> > helpers (un)lock_task_sighand instead of spin_lock_irqsave and
> > spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
> > suggested in the email referenced below.
>
> Ack.
> I'll take this once symbol issue is resolved ; thanks for your time.
>
> --
> Dominique
Noted.
Thanks.