netdev - Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 29 Jul 2015 09:22:01 +0200
From:	Oliver Neukum <oneukum@...e.com>
To:	Hayes Wang <hayeswang@...ltek.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	nic_swsd <nic_swsd@...ltek.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

On Wed, 2015-07-29 at 02:06 +0000, Hayes Wang wrote:
>  Oliver Neukum [mailto:oneukum@...e.com]
> > Sent: Tuesday, July 28, 2015 8:59 PM
> [...]
> > > > >  static void rtl8152_tx_timeout(struct net_device *netdev)  {
> > > > >         struct r8152 *tp = netdev_priv(netdev);
> > > > > -       int i;
> > > > >
> > > > >         netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> > > > > -       for (i = 0; i < RTL8152_MAX_TX; i++)
> > > > > -               usb_unlink_urb(tp->tx_info[i].urb);
> > > > > +
> > > > > +       usb_queue_reset_device(tp->intf);
> > > > > +       cancel_delayed_work(&tp->schedule);
> > > >
> > > > Sorry to bother you again, but this looks wrong.
> > > > You want to cancel first. There is no point in running any work
> > > > before the reset is done. It will undo any progress anyway.
> > >
> > > Excuse me. Do you mean I don't need cancel the other work because it
> > > wouldn't be run before the reset is finished?
> > 
> > No, whatever the other work will do, the reset will undo.
> 
> Excuse me. I don't understand why I couldn't use usb_queue_reset_device() directly.
> Why the reset will undo? 

Now, I think I got the reason for the confusion.

You are using cancel_delayed_work(&tp->schedule); after you queue
a reset. Therefore the order in which the work and the reset will
be executed is undefined. Usually the scheduled work will be canceled,
but not always.

That is not good.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html