RE: [PATCH V1 1/1] Serial: core: Add compat ioctl support

[Viken Dadhaniya]

Hi Greg,

Please find response inline.

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, December 22, 2022 11:19 AM
> To: Viken Dadhaniya (QUIC) <quic_vdadhani@xxxxxxxxxxx>
> Cc: jirislaby@xxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Mukesh Savaliya (QUIC) <quic_msavaliy@xxxxxxxxxxx>;
> Vijaya Krishna Nivarthi (Temp) (QUIC) <quic_vnivarth@xxxxxxxxxxx>; Visweswara
> Tanuku (QUIC) <quic_vtanuku@xxxxxxxxxxx>; Aniket RANDIVE (QUIC)
> <quic_arandive@xxxxxxxxxxx>
> Subject: Re: [PATCH V1 1/1] Serial: core: Add compat ioctl support
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> On Thu, Dec 22, 2022 at 10:19:25AM +0530, Viken Dadhaniya wrote:
> > Current serial core driver doesn't support compat_ioctl due to which
> > 32-bit application is not able to send ioctls to driver on a 64-bit
> > platform.
> 
> Are you sure?
> 
> > Added compat_ioctl support in serial core to handle ioctls from 32-bit
> > applications on a 64-bit platform.
> >
> > Signed-off-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
> > ---
> >  drivers/tty/serial/serial_core.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index b9fbbee598b8..5ffa0798db3b 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1601,6 +1601,22 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
> unsigned long arg)
> >       return ret;
> >  }
> >
> > +/**
> > + * uart_compat_ioctl: uart compat IOCTL function.
> > + * @tty: pointer to tty structure.
> > + * @cmd: command code passed by user-space.
> > + * @arg: argument  passed by user-space.
> > + *
> > + * This function will call normal uart IOCTL.
> > + *
> > + * Return: 0 for success, Negative number for error condition.
> 
> Why create kernel doc for a static function?  uart_ioctl() does not have this,
> right?
> 

I will remove documentation in V2 patch.

> > + */
> > +static long
> > +uart_compat_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned
> > +long arg) {
> > +     return (long)uart_ioctl(tty, cmd, arg); };
> 
> Really?  Why has this never shown up before as an issue?
>

We have only seen this problem with 32-bit applications running on 64-bit kernels.
For the first time, we are combining a 64-bit kernel with a 32-bit user space application.
 
> How was this tested?  What is currently broken that now works properly (or the
> other way around?)
>

We are running 32-bit user-space application on 64-bit kernel, and vendor specific ioctl command issued by application is not reaching the vendor driver.
Without these changes, the IOCTL call was returning from the tty framework rather than reaching to the vendor driver. IOCTL call is successfully reaching the vendor uart driver via tty framework after adding compat ioctl and functionality is working as expected and below is the dump stack with compat ioctl.

[ 1265.554002] Hardware name: Qualcomm Technologies, Inc. Monaco IDP V1.0 (DT)
[ 1265.554008] Call trace:
[ 1265.554011] dump_backtrace.cfi_jt+0x0/0x8
[ 1265.554023] show_stack+0x1c/0x2c
[ 1265.554032] dump_stack_lvl+0x80/0xc8
[ 1265.554041] dump_stack+0x1c/0x2c
[ 1265.554049] msm_geni_serial_ioctl+0x2cc/0x3d0 [msm_geni_serial]
[ 1265.554099] uart_ioctl+0x1a4/0x1d8
[ 1265.554109] uart_compat_ioctl+0x14/0x28
[ 1265.554117] tty_compat_ioctl+0x1a8/0x2dc
[ 1265.554125] __arm64_compat_sys_ioctl+0x158/0x1d0
[ 1265.554132] invoke_syscall+0x60/0x150
[ 1265.554140] el0_svc_common.llvm.3148309083493694862+0xc8/0x114
[ 1265.554148] do_el0_svc_compat+0x20/0x30
[ 1265.554154] el0_svc_compat+0x28/0x90
[ 1265.554162] el0t_32_sync_handler+0x7c/0xbc
[ 1265.554169] el0t_32_sync+0x1b8/0x1bc
 
> This patch implies that _every_ driver with an ioctl must create a
> compat_ioctl() callback, are you sure that is the case?
> 

this depends on user application and kernel version. for 32-bit application to run on 64-bit kernel, compat_ioctl is requried.

> thanks,
> 
> greg k-h

Thanks
Viken Dadhaniya