On 09/17/2013 10:24 AM, Josh Durgin wrote:
. . .
>>
>> OK, now I have a very broad (and too detailed) suggestion.
>> (I got a little carried away, sorry about that.)
>>
>> At this point there is only one IOCTL request that is handled
>> by this function. I know it doesn't match the general-purpose
>> structure of your typical ioctl routine, but unless you think
>> there are more ioctls that should be added, I think this could
>> be made simpler by structuring the function something like:
>
> I like this refactoring of it, but there are two minor issues
> present in the original patch too:
>
>> rbd_ioctl(...)
>> {
>> ret = 0;
>> int argval;
>> bool want_ro;
>>
>> if (cmd != BLKROSET)
>> return -EINVAL;
>
> According to block/ioctl.c, this should be -ENOTTY or -ENOIOCTLCOMMAND,
> not -EINVAL.
I wondered about that, and actually looked at it but I thought
I saw other ioctl functions returning -EINVAL so I stopped looking
and didn't mention anything.
>> if (get_user(argval, ...))
>> return -EFAULT;
>> want_ro = !!argval;
>>
>> spin_lock();
>> if (rbd_dev->mapping.read_only == want_ro)
>> goto err_unlock; /* No change, OK */
>>
>> if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !want_ro) {
>> ret = -EROFS;
>> goto err_unlock;
>> }
>>
>> /* Change requested; don't allow if already open */
>> if (rbd_dev->open_count) {
>> ret = -EBUSY;
>> goto err_unlock;
>> }
>>
>> rbd_dev->mapping.read_only = want_ro;
>> spin_unlock();
>> set_device_ro(bdev, want_ro ? 1 : 0);
>
> block/ioctl.c will already call set_device_ro() for us after this
> driver-specific handling completes successfully, so we don't need to
> call it here. Also, it appears the block layer has a bug in that
> it does the check for CAP_SYS_ADMIN *after* calling the driver-specific
> code for BLKROSET. So the driver-specific part could succeed, but the
> generic code could fail due to this later check without the driver
> knowing, possibly leaving the driver inconsistent with the block layer.
I wonder if that's intentional, but I doubt it. It predates
the original Linux-2.6.12-rc2 git commit.
But I agree with you, I think it's a bug. Do you plan to submit
a patch upstream to propose a fix?
-Alex
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
