On 2021/02/11 5:15, Shuah Khan wrote:

On 2/10/21 11:43 AM, Tetsuo Handa wrote:

quoted

On 2021/02/11 3:20, Shuah Khan wrote:

quoted

On 2/10/21 11:16 AM, Tetsuo Handa wrote:

quoted

On 2021/02/11 3:11, Shuah Khan wrote:

quoted

I would like to see to see a complete fix. This patch changes
kthread_get_run() to return NULL. Without adding handling for
NULL in the callers of kthread_get_run(), we will start seeing
problems.

What problems are you aware of?

The fact that driver doesn't cleanup after failing to create
the thread is a problem.

What are the cleanup functions?

When user-space requests attaching to a device, attach_store()
tries to attach the requested device. When kthread_get_run()
failure is ignored silently, and continue with call to
rh_port_connect(), user-space assumes attach is successful.
User thinks attach is successful.

The

  struct kthread_create_info *create = kmalloc(sizeof(*create), GFP_KERNEL);

in __kthread_create_on_node() never fails unless killed by the OOM killer
due to the "too small to fail" memory-allocation rule, and

  wait_for_completion_killable(&done)

in __kthread_create_on_node() never fails unless killed. Creating a kernel
thread as root user unlikely fails, and memory allocations by that kernel
thread also never fails due to the "too small to fail" memory-allocation rule.

Therefore, kthread_get_run() effectively fails only when current thread
which called attach_store() was killed. And

When and how will this attach failure gets reported to the
in this scenario?

if the current thread was killed, how can the failure get reported to
the user-space in this scenario?

Error handling for this case is no different from other error
paths in attach_store().

Please see error handling for other errors in attach_store().

Being "killed" means that user-space can never know the result
unlike other error paths in attach_store().

In this case the right error handling is to rewind the vdev
init and bail out returning error. This would include setting
vdev->ud.status to VDEV_ST_NULL.

If the user-space was killed, the kernel is responsible for offering
automatic cleanup which includes setting vdev->ud.status to VDEV_ST_NULL.

I found the following reproducer that tells me how attach
is triggered.
https://syzkaller.appspot.com/text?tag=ReproC&x=128506e4d00000

This reproducer (which is killed after 5 seconds from fork()) uses
only /sys/devices/platform/vhci_hcd.0/attach interface and never uses
detach interface. Detach and cleanup are up to automatic cleanup
offered by the kernel.

syzbot is helping us harden these paths, which is awesome.
Fixing these have to consider user api.

I you would like to fix this, please send me a complete fix.

If you want to handle the unlikely "__kthread_create_on_node() fails without
being killed" case, such change ( the drivers/usb/usbip/vhci_sysfs.c portion in
https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) should be a separate
patch. Since this patch declares "Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")",
this patch intends for the minimal change and does not want to do extra things.