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

[Hannes Reinecke]

On 10/31/2015 11:47 PM, Mike Snitzer wrote:
> On Sat, Oct 31 2015 at  3:07pm -0400,
> Paolo Bonzini <pbonzini at redhat.com> wrote:
> 
>>
>>
>> On 31/10/2015 19:13, Mike Snitzer wrote:
>>>> 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.
>>>
>>> [snip discussion of Christoph's patches]
>>>
>>> The point is scsi_verify_blk_ioctl() is saying the ioctl isn't valid.
>>> It has nothing to do with the existance of a bdev or not; but everything
>>> to do with the unprivledged user's request to issue an ioctl.
>>
>> ... but the call is skipped (and all ioctls are valid) if ti->len ==
>> i_size_read(bdev->bd_inode) >> SECTOR_SHIFT.  Therefore, until you have
>> the bdev you don't know which ioctls are valid, and you must assume all
>> of them are.  You can't do anything unsafe anyway until you have the
>> bdev.  This is the reasoning prior to Hannes's change.
> 
> Yes, with your commit ec8013be ("dm: do not forward ioctls from logical
> volumes to the underlying device") you added protections to disallow
> issuing ioctls to a partition that could impact the rest of the device.
> 
> Given that I can see why you're seizing on the "ti->len !=
> i_size_read(bdev->bd_inode) >> SECTOR_SHIFT" negative checks that gate
> the call to scsi_verify_blk_ioctl().
> 
>> Afterwards, you end up calling scsi_verify_blk_ioctl() when bdev ==
>> NULL.  If the future bdev satisfies the above condition on ti->len, this
>> means that ioctl(SG_IO) switches from ENOTTY to available.  Userspace is
>> clearly not expecting that.
> 
> 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 issues 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?
> 
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.

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?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)