One-element arrays are being deprecated[1]. Replace the one-element array
with a simple object of type compat_caddr_t: 'compat_caddr_t unused'[2],
once it seems this field is actually never used.
Also, update struct cdrom_generic_command in UAPI by adding an
anonimous union to avoid using the one-element array _reserved_.
[1] https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
[2] https://github.com/KSPP/linux/issues/86
Build-tested-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/lkml/5f76f5d0.qJ4t%2FHWuRzSW7bTa%[email protected]/
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
block/scsi_ioctl.c | 6 +++---
include/uapi/linux/cdrom.h | 5 ++++-
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 600e38cb69b2..2dfb699389df 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -643,7 +643,7 @@ struct compat_cdrom_generic_command {
unsigned char data_direction;
compat_int_t quiet;
compat_int_t timeout;
- compat_caddr_t reserved[1];
+ compat_caddr_t unused;
};
#endif
@@ -665,7 +665,7 @@ static int scsi_get_cdrom_generic_arg(struct cdrom_generic_command *cgc,
.data_direction = cgc32.data_direction,
.quiet = cgc32.quiet,
.timeout = cgc32.timeout,
- .reserved[0] = compat_ptr(cgc32.reserved[0]),
+ .unused = compat_ptr(cgc32.unused),
};
memcpy(&cgc->cmd, &cgc32.cmd, CDROM_PACKET_SIZE);
return 0;
@@ -690,7 +690,7 @@ static int scsi_put_cdrom_generic_arg(const struct cdrom_generic_command *cgc,
.data_direction = cgc->data_direction,
.quiet = cgc->quiet,
.timeout = cgc->timeout,
- .reserved[0] = (uintptr_t)(cgc->reserved[0]),
+ .unused = (uintptr_t)(cgc->unused),
};
memcpy(&cgc32.cmd, &cgc->cmd, CDROM_PACKET_SIZE);
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 2817230148fd..6c34f6e2f1f7 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -289,7 +289,10 @@ struct cdrom_generic_command
unsigned char data_direction;
int quiet;
int timeout;
- void __user *reserved[1]; /* unused, actually */
+ union {
+ void __user *reserved[1]; /* unused, actually */
+ void __user *unused;
+ };
};
/*
--
2.27.0
On 10/2/20 5:10 PM, Gustavo A. R. Silva wrote:
> diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
> index 2817230148fd..6c34f6e2f1f7 100644
> --- a/include/uapi/linux/cdrom.h
> +++ b/include/uapi/linux/cdrom.h
> @@ -289,7 +289,10 @@ struct cdrom_generic_command
> unsigned char data_direction;
> int quiet;
> int timeout;
> - void __user *reserved[1]; /* unused, actually */
> + union {
> + void __user *reserved[1]; /* unused, actually */
> + void __user *unused;
> + };
What's the point of this union, why not just turn it into
void * __user *unused;
?
--
Jens Axboe
On Fri, Oct 02, 2020 at 05:53:05PM -0600, Jens Axboe wrote:
> On 10/2/20 5:10 PM, Gustavo A. R. Silva wrote:
> > diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
> > index 2817230148fd..6c34f6e2f1f7 100644
> > --- a/include/uapi/linux/cdrom.h
> > +++ b/include/uapi/linux/cdrom.h
> > @@ -289,7 +289,10 @@ struct cdrom_generic_command
> > unsigned char data_direction;
> > int quiet;
> > int timeout;
> > - void __user *reserved[1]; /* unused, actually */
> > + union {
> > + void __user *reserved[1]; /* unused, actually */
> > + void __user *unused;
> > + };
>
> What's the point of this union, why not just turn it into
>
> void * __user *unused;
>
> ?
I just don't want to take any chances of breaking any user-space
application that, for some reason, may be considering that field.
--
Gustavo
On 10/2/20 6:03 PM, Gustavo A. R. Silva wrote:
> On Fri, Oct 02, 2020 at 05:53:05PM -0600, Jens Axboe wrote:
>> On 10/2/20 5:10 PM, Gustavo A. R. Silva wrote:
>>> diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
>>> index 2817230148fd..6c34f6e2f1f7 100644
>>> --- a/include/uapi/linux/cdrom.h
>>> +++ b/include/uapi/linux/cdrom.h
>>> @@ -289,7 +289,10 @@ struct cdrom_generic_command
>>> unsigned char data_direction;
>>> int quiet;
>>> int timeout;
>>> - void __user *reserved[1]; /* unused, actually */
>>> + union {
>>> + void __user *reserved[1]; /* unused, actually */
>>> + void __user *unused;
>>> + };
>>
>> What's the point of this union, why not just turn it into
>>
>> void * __user *unused;
>>
>> ?
>
> I just don't want to take any chances of breaking any user-space
> application that, for some reason, may be considering that field.
I guess that's a valid concern, who knows what applications are doing
to an ignored field.
I'll apply it, thanks.
--
Jens Axboe
On Fri, Oct 02, 2020 at 05:58:33PM -0600, Jens Axboe wrote:
> >>> - void __user *reserved[1]; /* unused, actually */
> >>> + union {
> >>> + void __user *reserved[1]; /* unused, actually */
> >>> + void __user *unused;
> >>> + };
> >>
> >> What's the point of this union, why not just turn it into
> >>
> >> void * __user *unused;
> >>
> >> ?
> >
> > I just don't want to take any chances of breaking any user-space
> > application that, for some reason, may be considering that field.
>
> I guess that's a valid concern, who knows what applications are doing
> to an ignored field.
>
> I'll apply it, thanks.
Awesome. :)
Thanks.
--
Gustavo
On Fri, Oct 02, 2020 at 06:10:33PM -0500, Gustavo A. R. Silva wrote:
> One-element arrays are being deprecated[1]. Replace the one-element array
> with a simple object of type compat_caddr_t: 'compat_caddr_t unused'[2],
> once it seems this field is actually never used.
They are only deprecated when abused as variable length array. That is
not the case here.