On 11/5/2024 4:50 AM, Felix Kuehling wrote: > > On 2024-10-31 22:35, Zhu Lingshan wrote: >> On 10/31/2024 11:30 PM, Felix Kuehling wrote: >>> On 2024-10-31 6:50, Zhu Lingshan wrote: >>>> The ioctl functions may fail, causing the args unreliable. >>>> Therefore, the args should not be copied to user space. >>>> >>>> The return code provides enough information for >>>> error handling in user space. >>>> >>>> This commit checks the return code of the ioctl functions >>>> and handles errors appropriately when they fail. >>> I have reviewed and rejected this patch before. My opinion has not changed. The existing code copies the ioctl arg structure back to user mode even in error cases because user mode needs additional information from that structure for some ioctls. >> how can the user space program distinguish the "good informational parameters" from the "bad default legacy parameters"? There can be other user space programs other than thunk. >> >> what if the user space program doing pulling mode, it can pull the args changes because ioctl is usually slower, our code should be robust. >> >> usually the return code provides enough information for the user space programs. > I don't understand your concern. Even without your patch, the failing ioctl still returns the error code to user mode. User mode can safely ignore additional information returned in the argument structure. You are raising concerns about performance or robustness. I don't see that either of those are negatively impacted by copying additional information in the argument struct to user mode. Still the questions: 1) how can the user space program distinguish the "good informational parameters" from the "bad default legacy parameters"? 2) what if the user space program doing pulling mode, pull the args before error code returned. Memory changes are usually faster than error code. > > You mention that there can be other user mode clients other than Thunk. That's true. E.g. rocm-gdb calls KFD ioctls directly. And it depends on some of the additional information about errors. If you know of other user mode clients that are broken by the current behaviour, please point them out. > > Before anything else, we do not break existing user mode. Your patch breaks that rule. There is really no room for discussion here. I'm not seeing any reasonable argument to even consider your proposal. If a user space program needs to read arguments to do error recovery, then it is a buggy user space program that should be fixed. Usually the error code provides enough information for error handling. Why our KFD user space are exceptive? Thanks Lingshan > > Regards, > Felix > >> Thanks >> Lingshan >>> Regards, >>> Felix >>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxx> >>>> --- >>>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>>> index 3e6b4736a7fe..a184ca0023b5 100644 >>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >>>> @@ -3327,6 +3327,8 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) >>>> } >>>> >>>> retcode = func(filep, process, kdata); >>>> + if (retcode) >>>> + goto err_retcode; >>>> >>>> if (cmd & IOC_OUT) >>>> if (copy_to_user((void __user *)arg, kdata, usize) != 0) >>>> @@ -3340,6 +3342,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) >>>> if (kdata != stack_kdata) >>>> kfree(kdata); >>>> >>>> +err_retcode: >>>> if (retcode) >>>> dev_dbg(kfd_device, "ioctl cmd (#0x%x), arg 0x%lx, ret = %d\n", >>>> nr, arg, retcode);
