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

[Mike Snitzer <snitzer@redhat.com>]

On Sat, Oct 31 2015 at  2:13P -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Sat, Oct 31 2015 at 11:33am -0400,
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > 
> > 
> > On 29/10/2015 14:18, Mike Snitzer wrote:
> > > > 4) dmesg shows that scsi_verify_blk_ioctl() failed for SG_IO (0x2285);
> > > >    it returns -ENOIOCTLCMD, later replaced with -ENOTTY in vfs_ioctl().
> > > > 
> > > >     $ dmesg
> > > >     <...>
> > > >     [] device-mapper: multipath: Failing path 65:144.
> > > >     [] device-mapper: multipath: Failing path 67:144.
> > > >     [] device-mapper: multipath: Failing path 65:224.
> > > >     [] device-mapper: multipath: Failing path 68:32.
> > > >     [] sgio_inquiry: sending ioctl 2285 to a partition!
> > > 
> > > So scsi_verify_blk_ioctl() considers the ioctl invalid.
> > 
> > But that's wrong, I think.  It's a false positive in
> > scsi_verify_blk_ioctl().
> > 
> > If the ioctl is valid when bdev becomes non-NULL (and it will be if
> > ti->len becomes equal to i_size_read(bdev->bd_inode) >> SECTOR_SHIFT),
> > you should not return -ENOIOCTLCMD aka ENOTTY, because userspace doesn't
> > think the ioctls can go away and come back.  So Hannes's patch broke the
> > userspace ABI. :(
> 
> Huh?  All that Hannes' patch did was add early verification of the ioctl
> if there are no paths, since: there is no point queueing an ioctl that
> is invalid.
> 
> But looking just now, Christoph's recent ioctl refactoring that I staged
> for 4.4 does seem to subtley revert Hannes' change:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.4&id=40cf639be1db8cc2b8183fe2ccd390ca77b90396
> With hch's change multipath_prepare_ioctl() will _not_ do early
> verification of the ioctl if no paths are available (and
> queue_if_no_path is configured).  Because the call to
> scsi_verify_blk_ioctl() was moved to dm_blk_ioctl() and is only called
> if the return is > 0 (again -ENOTCONN is being returned).
> 
> Not to mention hch's lifting of scsi_verify_blk_ioctl() into DM core's
> dm_blk_ioctl() -- likely motivated by not requiring all targets to do
> the call like they were doing -- should really be done as part of the
> new DM target .prepare_ioctl hook.
> 
> Christoph, I think your DM ioctl changes need more review/work.. which
> implies they'll very likely miss 4.4.. sorry.

This patch will maintain Hannes' commit a1989b3300935 (which I think is
correct!):

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index aaa6caa..ffea28f 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1562,6 +1562,16 @@ static int multipath_prepare_ioctl(struct dm_target *ti,
 
 	spin_unlock_irqrestore(&m->lock, flags);
 
+	/*
+	 * Only pass ioctls through if the device sizes match exactly.
+	 */
+	if (!*bdev || ti->len != i_size_read((*bdev)->bd_inode) >> SECTOR_SHIFT) {
+		/* not deferring to DM core to verify the ioctl */
+		int err = scsi_verify_blk_ioctl(NULL, cmd);
+		if (err)
+			r = err;
+	}
+
 	if (r == -ENOTCONN && !fatal_signal_pending(current)) {
 		spin_lock_irqsave(&m->lock, flags);
 		if (!m->current_pg) {
@@ -1574,11 +1584,6 @@ static int multipath_prepare_ioctl(struct dm_target *ti,
 		dm_table_run_md_queue_async(m->ti->table);
 	}
 
-	/*
-	 * Only pass ioctls through if the device sizes match exactly.
-	 */
-	if (!r && ti->len != i_size_read((*bdev)->bd_inode) >> SECTOR_SHIFT)
-		return 1;
 	return r;
 }
 
Christoph, I've folded this into your original commit 40cf639be1d I
referenced above, new commit is here:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.4&id=21a2807bc3ff0eec3e2ec35357a4c37d4bcbfd5b

But if you, Hannes or others disagree with this change I'll drop it for
4.4 and we'll have to revisit this later.