Bug 1876840

More details on how to reproduce it:

# lsblk
NAME                                                  MAJ:MIN RM  SIZE RO TYPE  MOUNTPOINT
...
sdk                                                     8:160  0  100G  0 disk  
└─360014054ad36a8bd0ee4dfa999a6674d                   253:6    0  100G  0 mpath 
  ├─47719c69--fcd6--4ebe--adb6--1080ea5d0c90-ids      253:15   0  128M  0 lvm   
  ├─47719c69--fcd6--4ebe--adb6--1080ea5d0c90-leases   253:18   0    2G  0 lvm   
  ├─47719c69--fcd6--4ebe--adb6--1080ea5d0c90-metadata 253:24   0  128M  0 lvm   
  ├─47719c69--fcd6--4ebe--adb6--1080ea5d0c90-inbox    253:25   0  128M  0 lvm   
  ├─47719c69--fcd6--4ebe--adb6--1080ea5d0c90-outbox   253:26   0  128M  0 lvm   
  ├─47719c69--fcd6--4ebe--adb6--1080ea5d0c90-xleases  253:27   0    1G  0 lvm   
  └─47719c69--fcd6--4ebe--adb6--1080ea5d0c90-master   253:28   0    1G  0 lvm   

# realpath /dev/47719c69-fcd6-4ebe-adb6-1080ea5d0c90/metadata 
/dev/dm-24

# ls -lh /dev/dm-24
brw-------. 1 vdsm qemu 253, 24 Sep 14 13:02 /dev/dm-24

# sudo -u vdsm -g qemu python3
Python 3.6.8 (default, Aug 18 2020, 08:33:21) 
[GCC 8.3.1 20191121 (Red Hat 8.3.1-5)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> fd = os.open("/dev/47719c69-fcd6-4ebe-adb6-1080ea5d0c90/metadata", os.O_RDWR | os.O_DIRECT)
>>> f = os.fdopen(fd, "rb", 0)

# journalctl -f
...
Sep 14 21:02:10 host4 kernel: device-mapper: core: python3: sending ioctl 5401 to DM device without required privilege.

Looking in python code, os.fdopen is another name or io.open, which
does this:

    PyObject *res = _PyObject_CallMethodId(raw, &PyId_isatty, NULL);

Which calls isatty() with the file descriptor.

So a simpler way to reproduce this would be:

>>> fd = os.open("/dev/47719c69-fcd6-4ebe-adb6-1080ea5d0c90/metadata", os.O_RDWR | os.O_DIRECT)
>>> os.isatty(fd)
False

This logs another warning:

# journalctl -f
...
Sep 14 21:09:36 host4 kernel: device-mapper: core: python3: sending ioctl 5401 to DM device without required privilege.

We have many such warnings, hiding real warnings:

# grep 'sending ioctl 5401 to DM device without required privilege' /var/log/messages | wc -l
219

Looking at the warnings on my testing setup, all the calls are from vdsm,
so adding RHV as dependent product.

Maybe there are some ioctls that benefit from this warning, but this
does not look helpful to anyone.

Introduced in:

commit e980f62353c697cbf0c4325e43df6e44399aeb64
Author: Christoph Hellwig <hch>
Date:   Sat Feb 4 10:45:03 2017 +0100

    dm: don't allow ioctls to targets that don't map to whole devices
    
    .. at least for unprivileged users.  Before we called into the SCSI
    ioctl code to allow excemptions for a few SCSI passthrough ioctls,
    but this is pretty unsafe and except for this call dm knows nothing
    about SCSI ioctls.
    
    As the SCSI ioctl code is now optional, we really don't want to
    drag it in for DM, and the exception is not very useful anyway.
    
    Signed-off-by: Christoph Hellwig <hch>
    Acked-by: Mike Snitzer <snitzer>
    Acked-by: Paolo Bonzini <pbonzini>
    Reviewed-by: Johannes Thumshirn <jthumshirn>
    Signed-off-by: Jens Axboe <axboe>

It warns for any ioctl if the caller lacks CAP_SYS_RAWIO:

+               if (!capable(CAP_SYS_RAWIO)) {
+                       DMWARN_LIMIT(
+       "%s: sending ioctl %x to DM device without required privilege.",
+                               current->comm, cmd);
+                       r = -ENOIOCTLCMD;

Grepping for all ioctls show much more warnings:

# egrep 'sending ioctl .+ to DM device without required privilege' /var/log/messages | awk '{print $11}' | sort | uniq -c
    309 5326
    219 5401
   1567 80306d02

# egrep 'sending ioctl 80306d02 to DM device without required privilege' /var/log/messages | awk '{print $8}' | sort | uniq -c
   1567 dd:

# egrep 'sending ioctl 5326 to DM device without required privilege' /var/log/messages | awk '{print $8}' | sort | uniq -c
    266 qemu-img:
     43 qemu-nbd:

*** Bug 1838453 has been marked as a duplicate of this bug. ***

*** Bug 1871065 has been marked as a duplicate of this bug. ***

The change in question (noted in comment#2) was made to report users that posed a security threat because they incorrectly issued ioctls to a subset of the device but the ioctl could affect the entire device.

(In reply to Nir Soffer from comment #1)
 
> Looking at the warnings on my testing setup, all the calls are from vdsm,
> so adding RHV as dependent product.
> 
> Maybe there are some ioctls that benefit from this warning, but this
> does not look helpful to anyone.

On the surface I can see why vdsm folks don't like these warnings for what _seems_ like a superficial call via python's isatty.
But needless actions have consequences (noise in logs).

I'm inclined to close this WONTFIX.  Please explain why it is DM's problem that userspace is initiating ioctls it shouldn't be?

(In reply to Mike Snitzer from comment #8)
> The change in question (noted in comment#2) was made to report users that
> posed a security threat because they incorrectly issued ioctls to a subset
> of the device but the ioctl could affect the entire device.

This is relevant only to ioctls that effect the entire device.

In the case of ioctl 5401, it does not change the device in any way.
Why should the kernel log a warning about this ioctl?

> (In reply to Nir Soffer from comment #1)
>  
> > Looking at the warnings on my testing setup, all the calls are from vdsm,
> > so adding RHV as dependent product.
> > 
> > Maybe there are some ioctls that benefit from this warning, but this
> > does not look helpful to anyone.
> 
> On the surface I can see why vdsm folks don't like these warnings for what
> _seems_ like a superficial call via python's isatty.
> But needless actions have consequences (noise in logs).

The usersapce code is doing any needless actions in this case.

Lets me show again the way to reproduce this:

>>> import os
>>> fd = os.open("/dev/47719c69-fcd6-4ebe-adb6-1080ea5d0c90/metadata", os.O_RDWR | os.O_DIRECT)

The only way to open a file with O_DIRECT (or some other combinations
of flags) is using os.open(). In the case of direct I/O, recently there
was no way to read from this file without wrapping the file descriptor
with a file object:

>>> f = os.fdopen(fd, "rb", 0)

With this file object, you can use f.readinto(buf) with aligned buffer
to read from the file.

So far, no needless actions.

os.fdopen checkss if fd is a tty, since it likes to use line buffering
for ttys. I don't think this is needless action.

> I'm inclined to close this WONTFIX.  Please explain why it is DM's problem
> that userspace is initiating ioctls it shouldn't be?

Device mapper is logging unhelpful warnings in cases when the warning is 
does not help anyone (read only ioctl). This causes lot of noise in user
logs and userspace have no way to avoid this log noise.

I'm answering only about ioctl 5401. There are other cases:

# egrep 'sending ioctl 80306d02 to DM device without required privilege' /var/log/messages | awk '{print $8}' | sort | uniq -c
   1567 dd:

# egrep 'sending ioctl 5326 to DM device without required privilege' /var/log/messages | awk '{print $8}' | sort | uniq -c
    266 qemu-img:
     43 qemu-nbd:

Kevin, do you know why qemu-img and qemu-nbd send ioctl 5326 when using
block devices? Is this something that can be avoided in qemu side?

In qemu-img/qemu-nbd, I think the call can come from:

static int cdrom_probe_device(const char *filename)
{
    ...
    /* Attempt to detect via a CDROM specific ioctl */
    ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
    ...
}

Which is used only in:

static BlockDriver bdrv_host_cdrom = {
    ...
    .bdrv_probe_device  = cdrom_probe_device,

bdrv_probe_device is used in:

static BlockDriver *find_hdev_driver(const char *filename)
{
    int score_max = 0, score;
    BlockDriver *drv = NULL, *d;

    QLIST_FOREACH(d, &bdrv_drivers, list) {
        if (d->bdrv_probe_device) {
            score = d->bdrv_probe_device(filename);
            if (score > score_max) {
                score_max = score;
                drv = d;
            }
        }
    }

    return drv;
}

Called from:

BlockDriver *bdrv_find_protocol(const char *filename,
                                bool allow_protocol_prefix,
                                Error **errp)
{
    ...
    drv1 = find_hdev_driver(filename);
    if (drv1) {
        return drv1;
    }
    ...

One of the calls is from qemu-img convert:

static int img_convert(int argc, char **argv)
{
    ...
       proto_drv = bdrv_find_protocol(out_filename, true, &local_err);


Another call is in qcow2.c:

static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
                                             const char *filename,
                                             QemuOpts *opts,
                                             Error **errp)
{
    ...
    /* Create and open the file (protocol layer) */
    ret = bdrv_create_file(filename, opts, errp);


Looks like this is the way qemu select the bdrv_host_cdrom
prtocol driver.

Looks reasonable to me, if qemu want to support reading from cdrom
it needs a way to detect cdrom, and getting drive status should
be harmless on anything, so there is no need to warn about this.

Indeed, that's the one. I don't see how to avoid it, and neither why it should be a problem. If QEMU gets an error code, it will correctly conclude that this is not the right device for the ioctl. Maybe the code that logs the warning should only warn on some specific ioctls that are known to be problematic (probably mainly SG_IO?).

In case of dd:

# egrep 'sending ioctl 80306d02 to DM device without required privilege' /var/log/messages | awk '{print $8}' | sort | uniq -c
   1567 dd:

I think comes from this code:

1688 static off_t
1689 skip_via_lseek (char const *filename, int fdesc, off_t offset, int whence)
1690 {
1691   struct mtget s1;
1692   struct mtget s2;
1693   bool got_original_tape_position = (ioctl (fdesc, MTIOCGET, &s1) == 0);
1694   /* known bad device type */
1695   /* && s.mt_type == MT_ISSCSI2 */
1696 
1697   off_t new_position = lseek (fdesc, offset, whence);
1698   if (0 <= new_position
1699       && got_original_tape_position
1700       && ioctl (fdesc, MTIOCGET, &s2) == 0
1701       && MT_SAME_POSITION (s1, s2))
1702     {
1703       if (status_level != STATUS_NONE)
1704         error (0, 0, _("warning: working around lseek kernel bug for file "
1705                        "(%s)\n  of mt_type=0x%0lx -- "
1706                        "see <sys/mtio.h> for the list of types"),
1707                filename, s2.mt_type + 0Lu);
1708       errno = 0;
1709       new_position = -1;
1710     }
1711 
1712   return new_position;
1713 }

This was added 19 years ago:
https://github.com/coreutils/coreutils/commit/24d1484e1c6a598e17a6069f3a0c5d882abd9535

As workaround for kernel bug with Exabyte SCSI tape drive accessed via
/dev/nst0 on both Linux-2.2.17 and Linux-2.4.16. This may not be 
relevant now.

This code is used when using skip= argument. Vdsm uses skip= to read
volumes metadata from a logical volume.

Karl, do you think we this workaround should be removed from dd so we
avoid the warning in the kernel?

I'm not coreutils maintainer ;-) CC: to Kamil Dudka.

(In reply to Nir Soffer from comment #12)
> Karl, do you think we this workaround should be removed from dd so we
> avoid the warning in the kernel?

Hopefully yes.  I have proposed a patch upstream to remove it:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44235

The code in question has been removed from GNU coreutils upstream:

https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.32-63-g4278e6615f

(In reply to Kamil Dudka from comment #15)
> The code in question has been removed from GNU coreutils upstream:
> 
> https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.32-63-
> g4278e6615f

Great, nice job by all sorting this out, feel free to clone this BZ for RHEL's coreutils.

But I'm closing this DM bz as WONTFIX.  If there is another ioctl warning storm in the future we can revisit.
In this instance the warnings served as an annoyingly useful canary in the coal mine that resulted in fixing the userspace code.

As always, feel free to reopen this BZ if anyone disagrees.

(In reply to Mike Snitzer from comment #16)
> (In reply to Kamil Dudka from comment #15)
> > The code in question has been removed from GNU coreutils upstream:
> > 
> > https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.32-63-
> > g4278e6615f
> 
> Great, nice job by all sorting this out,

Thanks, indeed!

> feel free to clone this BZ for
> RHEL's coreutils.
> 
> But I'm closing this DM bz as WONTFIX.  If there is another ioctl warning
> storm in the future we can revisit.

There is still one another already discussed above, not sure why this bug
should already be closed. See above, in comment 9. The case is in python's:

    os.fdopen(fd, "rb", 0)

But, I'd like to first get to a conclusion (well, the one we want :-) ) about
current bug, before converting it to a tracker for all the bad userspace code
in the world, so reopening:

(In reply to Mike Snitzer from comment #8)
> I'm inclined to close this WONTFIX.  Please explain why it is DM's problem
> that userspace is initiating ioctls it shouldn't be?

I'd like to propose another view:

Why would the kernel issue a useless warning?

If there is no security issue, why should we warn?

There are many ways for userspace code to do kernel calls that are essentially no-ops, and the kernel definitely does not warn about each of them. Why is this case different?

If there is a real security issue, the call should simply fail, IMO.

If it fails, any sane call that checks the return value will see that it
failed and can then decide what to do. Currently, the code calling such
"ioctls it shouldn't" has no means to know there is any problem. Only
*users*, not the "bad code", might notice it, if they bother checking logs.
They'll usually do this a long time after the fact, have a very hard time
correlating the warning to the specific code, along the way often wasting
this time when they actually wanted to find the solution for a *real*
problem they had - which is why they searched the logs.

TL;DR: IMO, if userspace issues a bad kernel call, it should fail. The
kernel does not need to warn about it, just fail the call. If it's not
bad enough to fail it, the kernel should not warn either, just allow.

Thanks!

(In reply to Yedidyah Bar David from comment #17)
> TL;DR: IMO, if userspace issues a bad kernel call, it should fail. The
> kernel does not need to warn about it, just fail the call. If it's not
> bad enough to fail it, the kernel should not warn either, just allow.

GNU coreutils upstream kind of shares the above attitude on this:

    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44235#8

So do I.  If kernel warned about all syscalls with insufficient permission, the log would be messy.

Also we still have an issue with python os.fdopen(), and in qemu-img
and qemu-nbd (comment 10).

We have a way to workaround this in python (using os.readv() instead
of os.fdopen())  But according to comment 11, we don't have a way to 
avoid the ioctl in qemu-img/nbd.

(In reply to Kamil Dudka from comment #18)
> (In reply to Yedidyah Bar David from comment #17)
> > TL;DR: IMO, if userspace issues a bad kernel call, it should fail. The
> > kernel does not need to warn about it, just fail the call. If it's not
> > bad enough to fail it, the kernel should not warn either, just allow.
> 
> GNU coreutils upstream kind of shares the above attitude on this:
> 
>     https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44235#8
> 
> So do I.  If kernel warned about all syscalls with insufficient permission,
> the log would be messy.

(In reply to Nir Soffer from comment #19)
> Also we still have an issue with python os.fdopen(), and in qemu-img
> and qemu-nbd (comment 10).
> 
> We have a way to workaround this in python (using os.readv() instead
> of os.fdopen())  But according to comment 11, we don't have a way to 
> avoid the ioctl in qemu-img/nbd.

OK, I'll just alter the error so the log isn't flooded (will use pr_debug_ratelimited, so that context for _why_ the failure occurred can be understood if debugging explicitly enabled).

Patch(es) available on kernel-4.18.0-283.el8.dt3

Hi Mike,
I finished pre-verification. You should add the bug to errata and change the status to ON_QA. ITM is 16. I need to verify the bug before Feb 22.
Thanks!

Patch(es) available on kernel-4.18.0-289.el8

*** Bug 1837465 has been marked as a duplicate of this bug. ***

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Important: kernel security, bug fix, and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2021:1578