linux-kernel - Re: [PATCH 2/2] target: Fix target_wait_for_sess

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: <1539236412.6150.18.camel@haakon3.daterainc.com>
Date:   Wed, 10 Oct 2018 22:40:12 -0700
From:   "Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     target-devel <target-devel@...r.kernel.org>,
        linux-scsi <linux-scsi@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Mike Christie <mchristi@...hat.com>,
        Hannes Reinecke <hare@...e.com>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        "Bryant G. Ly" <bryantly@...ux.vnet.ibm.com>,
        Bart Van Assche <bvanassche@....org>
Subject: Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with
 active signals

Hey Peter & Co,

On Wed, 2018-10-10 at 10:43 +0200, Peter Zijlstra wrote:
> On Wed, Oct 10, 2018 at 03:23:10AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@...ux-iscsi.org>
> > 
> > With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
> > signals will be pending for task_struct executing the normal session shutdown
> > and I/O quiesce code-path.
> > 
> > For example, iscsi-target and iser-target issue SIGINT to all kthreads as
> > part of session shutdown.  This has been the behaviour since day one.
> 
> Not knowing much context here; but does it make sense for those
> kthreads to handle signals, ever? Most kthreads should be fine with
> ignore_signals().
> 

iscsi-target + ib-isert uses SIGINT amongst dedicated rx/tx connection
kthreads to signal connection shutdown, requiring in-flight se_cmd I/O
descriptors to be quiesced before making forward progress to release
se_session.

By the point wait_event_lock_irq_timeout() is called in the example
here, one of the two rx/tx connection kthreads has been stopped, and the
other kthread is still processing shutdown.  So while historically the
pending SIGINTs where not cleared (or ignored) during shutdown at this
point, there is no reason why they could not be ignored for iscsi-target
+ ib-isert.

That said, pre commit 00d909a107 code always used wait_for_completion()
and ignored pending signals.  As-is target_wait_for_sess_cmds() is
called directly from fabric driver code and in one case also from
user-space via configfs_write_file(), so AFAICT it does need
TASK_UNINTERRUPTIBLE.