Re: IBM request to allow unprivledged ioctls [Was: Revert "dm mpath: fix stalls when handling invalid ioctls"]

[Mike Snitzer <snitzer@redhat.com>]

On Mon, Nov 02 2015 at  8:56am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 11/02/2015 02:31 PM, Mike Snitzer wrote:
> > On Mon, Nov 02 2015 at  2:28am -0500,
> > Hannes Reinecke <hare@suse.de> wrote:
> > 
> >> On 10/31/2015 11:47 PM, Mike Snitzer wrote:
> >>>
> >>> For Hannes, and in my head, it didn't matter if a future bdev satisfies
> >>> the length condition.  I don't think Hannes was trying to guard against
> >>> dangerous partition ioctls being issued by udev... but now I _do_
> >>> question what exactly _is_ the point of his commit 21a2807bc3f.  Which
> >>> invalid ioctls, from udev, did Hannes' change actually disallow?
> >>>
> > 
> > FYI, I meant s/21a2807bc3f/a1989b33/
> > 
> >> The reasoning is thus:
> >>
> >> With the original code we would need to wait for path activation
> >> before we would be able to figure out if the ioctl is valid.
> >> However, the callback to verify the ioctl is sd_ioctl(), which as
> >> a first step calls scsi_verify_ioctl().
> >> So my reasoning was that we can as well call scsi_verify_ioctl()
> >> early, and allow it to filter out known invalid ioctls.
> >> Then we wouldn't need to wait for path activation and return
> >> immediately.
> > 
> > Right, I understood that to be your intent.  And it seemed reasonible.
> > Unfortuntely, as we now know, scsi_verify_blk_ioctl() only really cares
> > to filter out ioctls that are invalid for partitions.
> > 
> > I'm still curious: which ioctls were being issued by udev that your
> > commit a1989b33 "fixed" (so udev workers didn't hang)?  Is the right fix
> > to modify udev to not issue such ioctls?  What was invalid about the
> > udev issued ioctls?
> > 
> Thing is, it's not udev itself which issues ioctl. It's the programs
> started by udev via the various rules (PROGRAM or IMPORT keywords).
> They might (and occasionally, will) issue ioctls, and we have no
> means of controlling them.
> 
> >> Incidentally, in the 'r == -ENOTCONN' case, we're waiting
> >> for path activation, but then just bail out with -ENOTCONN.
> >> As we're not resetting -ENOTCONN, where's the point in activate the
> >> paths here?
> >> Shouldn't we retry to figure out if -ENOTCONN is still true?
> > 
> > We do, in DM core, see _your_ commit that added this ;)
> > 6c182cd88d ("dm mpath: fix ioctl deadlock when no paths")
> > 
> Indeed.
> 
> But then the real question remains:
> 
> What is the 'correct' behaviour for ioctls when no path retry
> is active (or when no paths are present)?
> 
> Should we start path activation?
> If so, should we wait for path activation to finish, risking udev
> killing the worker for that event (udev has a built-in timeout of
> 120 seconds, which we might easily exceed when we need to activate
> paths for large installations or slow path activation ... just
> thinking of NetApp takeover/giveback cycle)?
> If we're not waiting for path activation, where would be the point
> in starting it, seeing that we're not actually interested in the result?

We only start it: if (r == -ENOTCONN && !fatal_signal_pending(current))

-ENOTCONN is set with:
if ((pgpath && m->queue_io) || (!pgpath && m->queue_if_no_path))

> And if we shouldn't start a path activation, what is the point of
> having code for it in the first place?

The point is we have reason to believe paths will be coming back or that
the user wants to queue_if_no_path.

> Couldn't we just rip it out altogether, and avoid much of the
> current unpleasantness?

Sorry, not seeing how you're getting there.