Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1575137443.5563.18.camel@HansenPartnership.com>
In-Reply-To: <1575137443.5563.18.camel@HansenPartnership.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 2 Dec 2019 13:57:57 -0800
Message-ID: <CAHk-=wjWNpPW91wyEj4FC4pOimWEUtLVb_RwQgB+9h2OO6ynyA@mail.gmail.com>
Subject: Re: [GIT PULL] first round of SCSI updates for the 5.4+ merge window
To:     James Bottomley <James.Bottomley@hansenpartnership.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Arnd Bergmann <arnd@arndb.de>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-scsi <linux-scsi@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Nov 30, 2019 at 10:10 AM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
>    The two major core
> changes are Al Viro's reworking of sg's handling of copy to/from user,
> Ming Lei's removal of the host busy counter to avoid contention in the
> multiqueue case and Damien Le Moal's fixing of residual tracking across
> error handling.

Math is hard. You say "The two major core changes are.." and then you
list _three_ changes.

Anyway, the sg copyin/out changes by Al conflicted fairly badly with
Arnd's compat_ioctl changes.

Al did

  c35a5cfb4150 ("scsi: sg: sg_read(): simplify reading ->pack_id of
userland sg_io_hdr_t")

which avoided doing a whole allocation of an 'sg_io_hdr_t' to just
read the one field of it.

But Arnd did

  98aaaec4a150 ("compat_ioctl: reimplement SG_IO handling")

which created a get_sg_io_hdr() helper that copied the 'sg_io_hdr_t'
from user space the right way for both compat and native, which
basically relied on the old approach.

So I effectively reverted Al's patch in order to take Arnd's patch in
the crazy sg legacy case that presumably nobody really cares about
anyway, since everybody should use SG_IO rather than the sg_read()
thing. But I know not everybody is.

I added a comment in that place:

              /*
               * This is stupid.
               *
               * We're copying the whole sg_io_hdr_t from user
               * space just to get the 'pack_id' field. But the
               * field is at different offsets for the compat
               * case, so we'll use "get_sg_io_hdr()" to copy
               * the whole thing and convert it.
               *
               * We could do something like just calculating the
               * offset based of 'in_compat_syscall()', but the
               * 'compat_sg_io_hdr' definition is in the wrong
               * place for that.
               */

since it turns out that the one 'pack_id' field we want does have the
same format in compat  mode as in native mode ("int" and
"compat_int_t" are the same), it's just at different offsets. But the
definition of 'compat_sg_io_hdr' isn't available in that place.

I'm leaving it to Al and Arnd to decide if they want to fix the
stupidity. I tried to make the minimally invasive merge resolution.

Al, Arnd? Comments?

It looks like linux-next punted on this entirely, and took Al's
simplified version that doesn't work with the compat case. Maybe I
should have done the same - if you use read() on the /dev/sg* device,
you deserve to get broken for the compat case. And it didn't
historically work anyway. But it was kind of sad to see how Arnd fixed
it, and then it got broken again.

I really really wish we could get rid of sg_read/sg_write() entirely,
and have SG_IO_SUBMIT and SG_IO_RECEIVE ioctl's that can handle the
queued cases that apparently some people need. Because the read/write
case really is disgusting.

                Linus