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

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jan 25, 2023 at 11:27:49AM +0000, Viken Dadhaniya wrote:
> Hi Greg,
> 
> Please find response inline.

As they should be :)

> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Thursday, December 22, 2022 11:19 AM
> > To: Viken Dadhaniya (QUIC) <quic_vdadhani@quicinc.com>
> > Cc: jirislaby@kernel.org; linux-serial@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Mukesh Savaliya (QUIC) <quic_msavaliy@quicinc.com>;
> > Vijaya Krishna Nivarthi (Temp) (QUIC) <quic_vnivarth@quicinc.com>; Visweswara
> > Tanuku (QUIC) <quic_vtanuku@quicinc.com>; Aniket RANDIVE (QUIC)
> > <quic_arandive@quicinc.com>
> > Subject: Re: [PATCH V1 1/1] Serial: core: Add compat ioctl support

That's horrible, please fix your email client.

> > 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.

What in-kernel tty ioctl is having a problem that a compat ioctl layer
is needed?  Let's fix that up.

As you know, we can't do anything about out-of-tree drivers.  Nor do you
want us to, so this really feels like a broken driver, it should NOT be
creating random new ioctls on the tty device node, that is NOT what it
is there for.

> [ 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]

This function is not in our kernel tree, so it looks to be a bug in that
driver, sorry.  Do NOT paper over out-of-tree driver bugs in the core
kernel (hint, are you sure you are even allowed to do that?)

> [ 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.

Again, what in-tree tty ioctls are affected by this?

thanks,

greg k-h