[PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Alexander Kapshuk]

Export symbol __lock_task_sighand, so it is accessible from code compiled
as modules.
This fixes the following modpost error:
ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!

Where __lock_task_sighand is called via lock_task_sighand in net/9p/client.c
See https://lore.kernel.org/lkml/20200620201456.14304-1-alexander.kapshuk@gmail.com/.

Signed-off-by: Alexander Kapshuk <alexander.kapshuk@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/TMTLPYU6A522JH2VCN3PNZVAP6EE5MDF/
---
 kernel/signal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/signal.c b/kernel/signal.c
index 5ca48cc5da76..2612b9098cbd 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1396,6 +1396,7 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,

 	return sighand;
 }
+EXPORT_SYMBOL(__lock_task_sighand);

 /*
  * send signal info to all the members of a group
--
2.27.0

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Dominique Martinet]

Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> Export symbol __lock_task_sighand, so it is accessible from code compiled
> as modules.
> This fixes the following modpost error:
> ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!

This can't fix something that's not broken (yet)! :)

I think it'd make more sense to describe why you think we should export
it, rather than describe a precise usecase e.g. justify why this would
be interesting to use from modules (e.g. it would help modules like 9p
take a lock on the current signal handler safely and cleanly through
lock_task_sighand())



Christian, Andrew - assuming this passes reviews from someone else I'm
not sure how to go forward with this; it'd be simpler for me if I could
take it in the 9p tree as I need it for the patch Alexander pointed at,
but I'm not normally touching any file outside of the 9p tree.
Is it better to let either of you take it normally (I think it'd be
you?) and wait for that to land, or can I take it in my tree for the
next merge window?



If we don't want to export it for some reason, I'm the one who suggested
using lock_task_sighand() so would be interested in how to go forward as
well; 9p probably shouldn't mess with signals at all... That part of the
code is especially bad as it makes the task unkillable in a weird way.

Actually I do have a patch that makes flush asynchronous and removes the
need for this altogether, maybe it's time I finish that patch serie
instead :P


Thanks,
-- 
Dominique

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Oleg Nesterov]

On 06/21, Alexander Kapshuk wrote:
>
> Export symbol __lock_task_sighand, so it is accessible from code compiled
> as modules.
> This fixes the following modpost error:
> ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!
>
> Where __lock_task_sighand is called via lock_task_sighand in net/9p/client.c
> See https://lore.kernel.org/lkml/20200620201456.14304-1-alexander.kapshuk@gmail.com/.

Why?

current->sighand is stable and can't go away. Unless "current" is exiting and
has already passed exit_notify(). So I don't think net/9p needs this helper.

However, the games with TIF_SIGPENDING doesn't look right in any case.

Oleg.

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Christian Brauner]

On Mon, Jun 22, 2020 at 08:25:28AM +0200, Oleg Nesterov wrote:
> On 06/21, Alexander Kapshuk wrote:
> >
> > Export symbol __lock_task_sighand, so it is accessible from code compiled
> > as modules.
> > This fixes the following modpost error:
> > ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!
> >
> > Where __lock_task_sighand is called via lock_task_sighand in net/9p/client.c
> > See https://lore.kernel.org/lkml/20200620201456.14304-1-alexander.kapshuk@gmail.com/.
> 
> Why?
> 
> current->sighand is stable and can't go away. Unless "current" is exiting and
> has already passed exit_notify(). So I don't think net/9p needs this helper.

From what I can gather from the thread (cf. [1]) that is linked in the
commit message the main motivation for all of this is sparse not being
happy and not some bug. (Maybe I'm not seeing something though.)

The patch itself linked here doesn't seem to buy anything. I agree with
Oleg. Afaict, lock_task_sighand() would only be needed here if the task
wouldn't be current. So maybe it should just be dropped from the series.

[1]: https://lore.kernel.org/lkml/20200620201456.14304-1-alexander.kapshuk@gmail.com/

Christian

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Christian Brauner]

On Sun, Jun 21, 2020 at 03:54:37PM +0200, Dominique Martinet wrote:
> Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> > Export symbol __lock_task_sighand, so it is accessible from code compiled
> > as modules.
> > This fixes the following modpost error:
> > ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!
> 
> This can't fix something that's not broken (yet)! :)
> 
> I think it'd make more sense to describe why you think we should export
> it, rather than describe a precise usecase e.g. justify why this would
> be interesting to use from modules (e.g. it would help modules like 9p
> take a lock on the current signal handler safely and cleanly through
> lock_task_sighand())
> 
> 
> 
> Christian, Andrew - assuming this passes reviews from someone else I'm
> not sure how to go forward with this; it'd be simpler for me if I could
> take it in the 9p tree as I need it for the patch Alexander pointed at,
> but I'm not normally touching any file outside of the 9p tree.
> Is it better to let either of you take it normally (I think it'd be
> you?) and wait for that to land, or can I take it in my tree for the
> next merge window?

Hm, I don't think the patch is really needed though; see my other mail. :)

Christian

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Alexander Kapshuk]

On Mon, Jun 22, 2020 at 11:40 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Sun, Jun 21, 2020 at 03:54:37PM +0200, Dominique Martinet wrote:
> > Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> > > Export symbol __lock_task_sighand, so it is accessible from code compiled
> > > as modules.
> > > This fixes the following modpost error:
> > > ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!
> >
> > This can't fix something that's not broken (yet)! :)
> >
> > I think it'd make more sense to describe why you think we should export
> > it, rather than describe a precise usecase e.g. justify why this would
> > be interesting to use from modules (e.g. it would help modules like 9p
> > take a lock on the current signal handler safely and cleanly through
> > lock_task_sighand())
> >
> >
> >
> > Christian, Andrew - assuming this passes reviews from someone else I'm
> > not sure how to go forward with this; it'd be simpler for me if I could
> > take it in the 9p tree as I need it for the patch Alexander pointed at,
> > but I'm not normally touching any file outside of the 9p tree.
> > Is it better to let either of you take it normally (I think it'd be
> > you?) and wait for that to land, or can I take it in my tree for the
> > next merge window?
>
> Hm, I don't think the patch is really needed though; see my other mail. :)
>
> Christian

Thanks Oleg and Christian for your feedback.
Based on what you both expressed, my patch that exported
__lock_task_sighand as well as the original patch to address a sparse
warning in net/9p/client.c
should be dropped and the warning considered a false positive.

I'll let Dominique have the final say on this.
Thanks.

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Dominique Martinet]

Christian Brauner wrote on Mon, Jun 22, 2020:
> On Mon, Jun 22, 2020 at 08:25:28AM +0200, Oleg Nesterov wrote:
>> current->sighand is stable and can't go away. Unless "current" is exiting and
>> has already passed exit_notify(). So I don't think net/9p needs this helper.
> 
> From what I can gather from the thread (cf. [1]) that is linked in the
> commit message the main motivation for all of this is sparse not being
> happy and not some bug. (Maybe I'm not seeing something though.)
> 
> The patch itself linked here doesn't seem to buy anything. I agree with
> Oleg. Afaict, lock_task_sighand() would only be needed here if the task
> wouldn't be current. So maybe it should just be dropped from the series.

Sure. I honestly have no idea on what guarantees we have from the task
being current here as opposed to any other task -- I guess that another
thread calling exit for exemple would have to wait?
What about the possibility of sighand being null that the function does
check, is that impossible for current as well?


Honestly not a part of the code I'm much familiar with, this all
predates my involvement with 9p by a fair bit...

Anyway, not particularily fussy on this, it just looked like "the right
way" to lock a task signal handler among the few common patterns I could
see; I think it would make sense to just convert all such locks to a
single pattern for a maintenance pov but it's much more work than I'm
willing to do.
I'll just drop the patch :)


>> However, the games with TIF_SIGPENDING doesn't look right in any
>> case.

I definitely agree with this, hence my comment about an old patchset
that will remove these eventually, but while I did send the patches over
a year ago I never took them up due to lack of proper testing.
It's been something people regularily complained about that it makes the
task unkillable in a weird way and many tools like syzbot don't like it
(and potentially users who try ^C won't either)

I guess I'll try to find some time to finish that instead... Will be
more useful than trying to wrap my head around all of that :P


Thanks!
-- 
Dominique

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Oleg Nesterov]

On 06/22, Dominique Martinet wrote:
>
> What about the possibility of sighand being null that the function does
> check, is that impossible for current as well?

It is only possible if "current" has already exited and passed exit_notify(),
iow if it is already a zombie and can be (auto)reaped.

Oleg.

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Christian Brauner]

On Mon, Jun 22, 2020 at 12:24:01PM +0200, Dominique Martinet wrote:
> Christian Brauner wrote on Mon, Jun 22, 2020:
> > On Mon, Jun 22, 2020 at 08:25:28AM +0200, Oleg Nesterov wrote:
> >> current->sighand is stable and can't go away. Unless "current" is exiting and
> >> has already passed exit_notify(). So I don't think net/9p needs this helper.
> > 
> > From what I can gather from the thread (cf. [1]) that is linked in the
> > commit message the main motivation for all of this is sparse not being
> > happy and not some bug. (Maybe I'm not seeing something though.)
> > 
> > The patch itself linked here doesn't seem to buy anything. I agree with
> > Oleg. Afaict, lock_task_sighand() would only be needed here if the task
> > wouldn't be current. So maybe it should just be dropped from the series.
> 
> Sure. I honestly have no idea on what guarantees we have from the task
> being current here as opposed to any other task -- I guess that another
> thread calling exit for exemple would have to wait?

When a thread in a non-trivial thread-group (sorry for the math
reference :)) execs it'll unshare its struct sighand. The new struct
sighand will be assigned using rcu_assign_pointer() so afaik (Paul or
Oleg can yell at me if I'm talking nonsense) any prior callers will see
the prior sighand value.

> What about the possibility of sighand being null that the function does
> check, is that impossible for current as well?

See above, I think that's impossible.

> 
> 
> Honestly not a part of the code I'm much familiar with, this all
> predates my involvement with 9p by a fair bit...

We can't be experts in everything. :)

Christian

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Oleg Nesterov]

On 06/22, Christian Brauner wrote:
>
> On Mon, Jun 22, 2020 at 12:24:01PM +0200, Dominique Martinet wrote:
> > Christian Brauner wrote on Mon, Jun 22, 2020:
> > > On Mon, Jun 22, 2020 at 08:25:28AM +0200, Oleg Nesterov wrote:
> > >> current->sighand is stable and can't go away. Unless "current" is exiting and
> > >> has already passed exit_notify(). So I don't think net/9p needs this helper.
> > >
> > > From what I can gather from the thread (cf. [1]) that is linked in the
> > > commit message the main motivation for all of this is sparse not being
> > > happy and not some bug. (Maybe I'm not seeing something though.)
> > >
> > > The patch itself linked here doesn't seem to buy anything. I agree with
> > > Oleg. Afaict, lock_task_sighand() would only be needed here if the task
> > > wouldn't be current. So maybe it should just be dropped from the series.
> >
> > Sure. I honestly have no idea on what guarantees we have from the task
> > being current here as opposed to any other task -- I guess that another
> > thread calling exit for exemple would have to wait?
>
> When a thread in a non-trivial thread-group (sorry for the math
> reference :)) execs it'll unshare its struct sighand.

Well, not really...

The execing threads will kill other other threads, then it will check
if ->sighand should be unshared. The latter is very unlikely, I don't
think CLONE_SIGHAND without CLONE_THREAD is actually used today.

But this doesn't really matter. I mean, even if you race with another
thread doing exec/exit/whatever, current->sighand is stable. Unless, again,
current has already exited (called exit_notify()).

> The new struct
> sighand will be assigned using rcu_assign_pointer() so afaik (Paul or
> Oleg can yell at me if I'm talking nonsense) any prior callers will see
> the prior sighand value.

Yes, but see above.

If tsk is not current, then (in general) it is not safe to use tsk->sighand
directly. It can can be changed by exec (as you explained), or you can hit
tsk->sighand == NULL if you race with exit.

Oleg.

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Christian Brauner]

On Mon, Jun 22, 2020 at 02:03:00PM +0200, Oleg Nesterov wrote:
> On 06/22, Christian Brauner wrote:
> >
> > On Mon, Jun 22, 2020 at 12:24:01PM +0200, Dominique Martinet wrote:
> > > Christian Brauner wrote on Mon, Jun 22, 2020:
> > > > On Mon, Jun 22, 2020 at 08:25:28AM +0200, Oleg Nesterov wrote:
> > > >> current->sighand is stable and can't go away. Unless "current" is exiting and
> > > >> has already passed exit_notify(). So I don't think net/9p needs this helper.
> > > >
> > > > From what I can gather from the thread (cf. [1]) that is linked in the
> > > > commit message the main motivation for all of this is sparse not being
> > > > happy and not some bug. (Maybe I'm not seeing something though.)
> > > >
> > > > The patch itself linked here doesn't seem to buy anything. I agree with
> > > > Oleg. Afaict, lock_task_sighand() would only be needed here if the task
> > > > wouldn't be current. So maybe it should just be dropped from the series.
> > >
> > > Sure. I honestly have no idea on what guarantees we have from the task
> > > being current here as opposed to any other task -- I guess that another
> > > thread calling exit for exemple would have to wait?
> >
> > When a thread in a non-trivial thread-group (sorry for the math
> > reference :)) execs it'll unshare its struct sighand.
> 
> Well, not really...
> 
> The execing threads will kill other other threads, then it will check

I know but I didn't want to go into that much detail but you're right of
course! :)

> if ->sighand should be unshared. The latter is very unlikely, I don't
> think CLONE_SIGHAND without CLONE_THREAD is actually used today.

It is a supported case however unlikely. I just tried to answer
Dominique's specific question pointing out that even in that unlikely
case sighand_struct is stable.

Just as an fyi, CLONE_SIGHAND with CLONE_VM but without CLONE_THREAD is
actually used quite a bit, e.g. in newlib, in stress-ng, and in criu.
Actually for some use-cases with userfaultfd if you want to handle
pagefaults in the child, you'd want CLONE_VM which enforces
CLONE_SIGHAND so that would be another use-case afaict.

And honestly, quite often this combo is used in helper processes that
share as much context as possible without CLONE_THREAD to do as little
work as possible in terms of allocations and so on in the kernel.
(Another interesting use-case could arise with CLONE_SIGHAND +
 CLONE_CLEAR_SIGHAND as it allows you to reset both the parent's and
 child's signal handler in one shot.)

> 
> But this doesn't really matter. I mean, even if you race with another
> thread doing exec/exit/whatever, current->sighand is stable. Unless, again,
> current has already exited (called exit_notify()).
> 
> > The new struct
> > sighand will be assigned using rcu_assign_pointer() so afaik (Paul or
> > Oleg can yell at me if I'm talking nonsense) any prior callers will see
> > the prior sighand value.
> 
> Yes, but see above.
> 
> If tsk is not current, then (in general) it is not safe to use tsk->sighand
> directly. It can can be changed by exec (as you explained), or you can hit
> tsk->sighand == NULL if you race with exit.
> 
> Oleg.
> 

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Oleg Nesterov]

On 06/22, Christian Brauner wrote:
>
> It is a supported case however unlikely. I just tried to answer
> Dominique's specific question pointing out that even in that unlikely
> case sighand_struct is stable.

I too tried to say this, but apparently just added more confusion ;)

> Just as an fyi, CLONE_SIGHAND with CLONE_VM but without CLONE_THREAD is
> actually used quite a bit, e.g. in newlib, in stress-ng, and in criu.

OK,

> you'd want CLONE_VM which enforces
> CLONE_SIGHAND so that would be another use-case afaict.

Cough no ;) CLONE_SIGHAND requires CLONE_VM, not vice versa.

> > But this doesn't really matter. I mean, even if you race with another
> > thread doing exec/exit/whatever, current->sighand is stable. Unless, again,
> > current has already exited (called exit_notify()).

Oleg.

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Christian Brauner]

On Mon, Jun 22, 2020 at 03:01:55PM +0200, Oleg Nesterov wrote:
> On 06/22, Christian Brauner wrote:
> >
> > It is a supported case however unlikely. I just tried to answer
> > Dominique's specific question pointing out that even in that unlikely
> > case sighand_struct is stable.
> 
> I too tried to say this, but apparently just added more confusion ;)
> 
> > Just as an fyi, CLONE_SIGHAND with CLONE_VM but without CLONE_THREAD is
> > actually used quite a bit, e.g. in newlib, in stress-ng, and in criu.
> 
> OK,
> 
> > you'd want CLONE_VM which enforces
> > CLONE_SIGHAND so that would be another use-case afaict.
> 
> Cough no ;) CLONE_SIGHAND requires CLONE_VM, not vice versa.

Oh, you're right. I was thinking of ksys_unshare() here. :)

	/*
	 * If unsharing vm, must also unshare signal handlers.
	 */
	if (unshare_flags & CLONE_VM)
		unshare_flags |= CLONE_SIGHAND;

Thanks!
Christian