Re: [PATCH v3 17/20] sg: add sg_iosubmit_v3 and sg_ioreceive_v3 ioctls

[James Bottomley <jejb@linux.vnet.ibm.com>]

On Mon, 2019-08-12 at 12:14 -0400, Tony Battersby wrote:
> On 8/12/19 11:37 AM, Douglas Gilbert wrote:
> > On 2019-08-09 7:15 p.m., James Bottomley wrote:
> > > On Wed, 2019-08-07 at 13:42 +0200, Douglas Gilbert wrote:
> > > > Add ioctl(SG_IOSUBMIT_V3) and ioctl(SG_IORECEIVE_V3). These
> > > > ioctls
> > > > are meant to be (almost) drop-in replacements for the
> > > > write()/read()
> > > > async version 3 interface. They only accept the version 3
> > > > interface.
> > > 
> > > I don't think we should do this at all.  Anyone who wants to use
> > > the
> > > new async interfaces should use the v4 headers.  As Tony
> > > Battersby
> > > already said, the legacy v3 users aren't going to update, so
> > > there's no
> > > point at all introducing new interfaces for v3.  We simply keep
> > > the v3
> > > only read/write interface until there are no users left and it
> > > can be
> > > eliminated.
> > 
> > Tony Battersby wrote [20190809]:
> >    "Actually I used the asynchronous write()/read()/poll() sg
> > interface to
> >    implement RAID-like functionality for tape drives and medium
> > changers,
> >    in a commercial product that has been around since 2002.  These
> > days our
> >    products use a lot more disk I/O than tape I/O, so I don't write
> > much
> >    new code using the sg interface anymore, although that code is
> > still
> >    there and has to be maintained as needed.  So I don't have any
> > immediate
> >    plans to use any of the new sgv4 features being introduced, and
> >    unfortunately I am way too busy to even give it a good
> > review..."
> > 
> > That is quoted in full his post. And here is the only other post
> > from
> > Tony I can find on this subject, again quoted in full [20190808]:
> > 
> >    "One of the reasons ioctls have a bad reputation is because they
> > can be
> >    used to implement poorly-thought-out interfaces.  So kernel
> > maintainers
> >    push back on adding new ioctls.  But the push back isn't about
> > the
> >    number of ioctls, it is about the poor interfaces.  My advice
> > was that
> >    in general, to implement a given API, it would be better to add
> > more
> >    ioctls with a simple interface for each one rather than to add
> > fewer
> >    extremely complex multiplexing ioctls."
> > 
> > Call me biased but I believe that taken together those posts
> > support
> > what I am proposing. And I can _not_ see how you deduce: "so
> > there's
> > no point at all introducing new interfaces for v3" in reference to
> > Tony's posts.
> > 
> > 
> > As I stated in a previous post, I do not consider the sg v3
> > interface
> > as legacy. Where simply implemented, I am prepared to implement new
> > features on both the sg v3 and v4 interfaces. One example of this
> > is
> > doing command timing in nanoseconds rather than the current
> > default,
> > which is timing in milliseconds. There is also the new option of
> > not
> > doing any command timing at all. In my current implementation it
> > would
> > actually be more code to implement that for the v4 interface but
> > not
> > for the v3 interface.
> > 
> > Replicating my argument from a previous post:
> > If the kernel had an API mapping layer that was sensitive to file
> > descriptors of a "special file" (e.g. "/dev/sg3") then it could
> > map:
> >      write(sg_fd, &sg_io_v3_obj, sizeof(sg_io_v3_obj))
> > to
> >      ioctl(sg_fd, SG_IOSUBMIT_V3, &sg_io_v3_obj)
> > 
> > Plus a similar mapping for read() to ioctl(SG_IORECEIVE_V3). If
> > such
> > a mapping did exist and was transparent to the user then write()
> > and read() could be retired from the sg driver.  And I assume that
> > would get a thumbs up from the kernel security folk.
> > 
> 
> FWIW, my employer will probably continue to use the async sg v3
> interface for a long time.  If the read/write syscalls are a security
> problem, and if we had ioctl()s that are mostly a drop-in replacement
> for them, then we could convert our products over to the new ioctl()s
> on our next kernel upgrade without too much work (our products are
> embedded devices, so we control the whole software stack).  So if you
> plan to deprecate the read/write syscall interface anytime soon, then
> having drop-in replacement ioctl()s would be beneficial, even if it
> can't be done transparently as Doug suggests.

So far we've mitigated the security threat by withdrawing the v4
r/w interface which you don't use and keeping the sg nodes root only
for v3 r/w.  Unless we get another security incident based on them, as
long as the use case doesn't expand, I think the prior issue is pretty
nasty but contained to root who should know what they're doing, so
there's no pressing need to remove it.

Given that shifting to ioctls or a different async interface would be
development anyway, is there a solid reason you couldn't also shift to
v4 if you do that?  I know all the field names changed but for a
standard SCSI command it should be very similar to v3.

James