Re: [RFC 3/3] nvme : Add ioctl to query nvme attributes

[Joel Granados <j.granados@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

On Fri, Oct 28, 2022 at 10:22:35AM -0600, Keith Busch wrote:
> On Fri, Oct 28, 2022 at 05:52:30PM +0200, Joel Granados wrote:
> > On Fri, Oct 28, 2022 at 08:52:01AM -0600, Keith Busch wrote:
> > > On Fri, Oct 28, 2022 at 12:38:27PM +0200, Joel Granados wrote:
> > > > 3. And the variables that are not in struct nvme_ctrl. Here, we have not
> > > >    option but to make an IO.
> > > 
> > > You do have another option: we have historically added new fields to
> > > nvme_ctrl to cache parameters that are repeatedly referenced in other
> > > contexts, and this usage appears to qualify.
> > 
> > Agreed. That is yet another option. Will do that for my V1.
> > Thx for the feedback
> 
> Just for the record, I don't really like this approach, but I don't have
> a better suggestion right now other than opening up the admin queue or
> exporting a ton of sysfs attributes (and each carry a different set of
> concerns..).
> 
> My concern here is that drives implementing new specs may have new
> constraints that applications need to know, but older kernels won't
> accomodate. You are handling this by error'ing out the query, so it
> relies on the application implementing a sane fallback (assuming such a
> fallback for future attributes even exists). This just does not feel
> very future-proof.

There might be an additional route to handle the case where the user has
new kernel headers and is running the ioctl on an older kernel: We can
use copy_struct_from_user f5a1a536fa14 ("lib: introduce
copy_struct_from_user() helper").
In summary it has a clear behavior/assumptions for the three possible
cases:
1. When kernel and user space have the same header version it just
   copies the struct.
2. When the kernel has a higher version of the struct, it only copies
   what the user space requested in argsz.
3. When user has an old kernel (The case that you are worried about) it
   will only fill up the members that the kernel knows about provided
   that members that are not contained in the kernel struct are zeroed
   out.

What do you think?

I did not implement this at the outset because I was wanting to avoid
the additional call to copy_struct_from_user. But if this provides a
suitable solution for you case, then it is trivial to include it in the
patch.

Best

Joel



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]