linux-kernel - Re: [PATCH AUTOSEL 5.2 039/249] signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH2r5mu9ncYa1WTHuuMEk3=4TU5-RBH6nBKME4Bm+dntOtORTQ@mail.gmail.com>
Date:   Tue, 23 Jul 2019 19:29:10 -0500
From:   Steve French <smfrench@...il.com>
To:     ronnie sahlberg <ronniesahlberg@...il.com>
Cc:     Sasha Levin <sashal@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Stable <stable@...r.kernel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Namjae Jeon <namjae.jeon@...sung.com>,
        Jeff Layton <jlayton@...marydata.com>,
        linux-cifs <linux-cifs@...r.kernel.org>
Subject: Re: [PATCH AUTOSEL 5.2 039/249] signal/cifs: Fix cifs_put_tcp_session
 to call send_sig instead of force_sig

Very easy to see what caused the regression with this global change:

mount (which launches "cifsd" thread to read the socket)
umount (which kills the "cifsd" thread)
rmmod   (rmmod now fails since "cifsd" thread is still active)

mount launches a thread to read from the socket ("cifsd")
umount is supposed to kill that thread (but with the patch
"signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of
force_sig" that no longer works).  So the regression is that after
unmount you still see the "cifsd" thread, and the reason that cifsd
thread is still around is that that patch no longer force kills the
process (see line 2652 of fs/cifs/connect.c) which regresses module
removal.

-               force_sig(SIGKILL, task);
+               send_sig(SIGKILL, task, 1);

The comment in the changeset indicates "The signal SIGKILL can not be
ignored" but obviously it can be ignored - at least on 5.3-rc1 it is
being ignored.

If send_sig(SIGKILL ...) doesn't work and if force_sig(SIGKILL, task)
is removed and no longer possible - how do we kill a helper process
...

On Tue, Jul 23, 2019 at 6:20 PM ronnie sahlberg
<ronniesahlberg@...il.com> wrote:
>
> On Tue, Jul 16, 2019 at 1:15 AM Sasha Levin <sashal@...nel.org> wrote:
> >
> > From: "Eric W. Biederman" <ebiederm@...ssion.com>
> >
> > [ Upstream commit 72abe3bcf0911d69b46c1e8bdb5612675e0ac42c ]
> >
> > The locking in force_sig_info is not prepared to deal with a task that
> > exits or execs (as sighand may change).  The is not a locking problem
> > in force_sig as force_sig is only built to handle synchronous
> > exceptions.
> >
> > Further the function force_sig_info changes the signal state if the
> > signal is ignored, or blocked or if SIGNAL_UNKILLABLE will prevent the
> > delivery of the signal.  The signal SIGKILL can not be ignored and can
> > not be blocked and SIGNAL_UNKILLABLE won't prevent it from being
> > delivered.
> >
> > So using force_sig rather than send_sig for SIGKILL is confusing
> > and pointless.
> >
> > Because it won't impact the sending of the signal and and because
> > using force_sig is wrong, replace force_sig with send_sig.
>
> I think this patch broke the cifs module.
> The issue is that the use count is now not updated properly and thus
> it is no longer possible to
> rmmod the module.
>
>
> >
> > Cc: Namjae Jeon <namjae.jeon@...sung.com>
> > Cc: Jeff Layton <jlayton@...marydata.com>
> > Cc: Steve French <smfrench@...il.com>
> > Fixes: a5c3e1c725af ("Revert "cifs: No need to send SIGKILL to demux_thread during umount"")
> > Fixes: e7ddee9037e7 ("cifs: disable sharing session and tcon and add new TCP sharing code")
> > Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> > Signed-off-by: Sasha Levin <sashal@...nel.org>
> > ---
> >  fs/cifs/connect.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 8dd6637a3cbb..714a359c7c8d 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -2631,7 +2631,7 @@ cifs_put_tcp_session(struct TCP_Server_Info *server, int from_reconnect)
> >
> >         task = xchg(&server->tsk, NULL);
> >         if (task)
> > -               force_sig(SIGKILL, task);
> > +               send_sig(SIGKILL, task, 1);
> >  }
> >
> >  static struct TCP_Server_Info *
> > --
> > 2.20.1
> >



-- 
Thanks,

Steve