circular submissions in cdc-wdm and how to break them on disconnect

Hi,

you have moved kill_urbs() below

        cancel_work_sync(&desc->rxwork);
        cancel_work_sync(&desc->service_outs_intr);

to close a race, as

rv = usb_submit_urb(desc->response, GFP_KERNEL);

in service_outstanding_interrupt() would submit the response URB,
right? Unfortunately we have in wdm_in_callback() the following code path

        if (desc->rerr) {
                /*
                 * Since there was an error, userspace may decide to not read
                 * any data after poll'ing.
                 * We should respond to further attempts from the device to send
                 * data, so that we can get unstuck.
                 */
                schedule_work(&desc->service_outs_intr);

It looks to me like we have a circular dependency here and this needs some
change to break. What do you think about the attached patch?

	Regards
		Oliver

On 2021/01/22 0:30, Oliver Neukum wrote:

Hi,

you have moved kill_urbs() below

        cancel_work_sync(&desc->rxwork);
        cancel_work_sync(&desc->service_outs_intr);

to close a race, as

rv = usb_submit_urb(desc->response, GFP_KERNEL);

in service_outstanding_interrupt() would submit the response URB,
right?

Right. Shouldn't remaining

  kill_urbs(desc);
  cancel_work_sync(&desc->rxwork);
  cancel_work_sync(&desc->service_outs_intr);

sequence in wdm_suspend() and wdm_pre_reset() be updated as well?

       Unfortunately we have in wdm_in_callback() the following code path

        if (desc->rerr) {
                /*
                 * Since there was an error, userspace may decide to not read
                 * any data after poll'ing.
                 * We should respond to further attempts from the device to send
                 * data, so that we can get unstuck.
                 */
                schedule_work(&desc->service_outs_intr);

It looks to me like we have a circular dependency here and this needs some
change to break. What do you think about the attached patch?

I don't know how poisoning works. But why can't we simply use test_bit() on
WDM_SUSPENDING/WDM_RESETTING/WDM_DISCONNECTING flags, for schedule_work() in
wdm_in_callback() is called with desc->iuspin (which serializes setting of
these flags) held.

By the way, since someone might interpret "broken" as "out of order / not working",
I expect not using "This needs to be broken." in the commit message. There would be
some better idiom.

Am Samstag, den 23.01.2021, 11:57 +0900 schrieb Tetsuo Handa:

On 2021/01/22 0:30, Oliver Neukum wrote:

Hi,

Right. Shouldn't remaining

  kill_urbs(desc);
  cancel_work_sync(&desc->rxwork);
  cancel_work_sync(&desc->service_outs_intr);

sequence in wdm_suspend() and wdm_pre_reset() be updated as well?

Yes, they should.

quoted

       Unfortunately we have in wdm_in_callback() the following code path

        if (desc->rerr) {
                /*
                 * Since there was an error, userspace may decide to not read
                 * any data after poll'ing.
                 * We should respond to further attempts from the device to send
                 * data, so that we can get unstuck.
                 */
                schedule_work(&desc->service_outs_intr);

It looks to me like we have a circular dependency here and this needs some
change to break. What do you think about the attached patch?

I don't know how poisoning works. But why can't we simply use test_bit() on

It makes subsequent usb_submit_urb() fail.

WDM_SUSPENDING/WDM_RESETTING/WDM_DISCONNECTING flags, for schedule_work() in
wdm_in_callback() is called with desc->iuspin (which serializes setting of
these flags) held.

In theory this could be done, yet that would take three additional
tests as opposed to the test for poisoning that usbcore does anyway.

By the way, since someone might interpret "broken" as "out of order / not working",
I expect not using "This needs to be broken." in the commit message. There would be
some better idiom.

Right. I changed the message.

Could you test whether the attached patch fixes your issue?

	Regards
		Oliver

On 2021/02/18 21:55, Oliver Neukum wrote:

Could you test whether the attached patch fixes your issue?

"INFO: task hung in wdm_flush" was fixed on 2020/11/16 12:12, and
as far as I know syzbot is not reporting any problem with this driver.
I don't have issues regardless of your patch. I can't test your patch.

URBs with thos callbacks. This needs to be blocked, stopped

s/thos/those/