netdev - Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

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]

Message-ID: <CAPopfEXz+HPdwWh_GQ2WzMnHh5DuXTse78NAU=-iN15wv-0C5g@mail.gmail.com>
Date:	Mon, 5 Sep 2011 11:29:05 -0500
From:	Jim Wylder <jwylder1@...orola.com>
To:	Oliver Neukum <oliver@...kum.org>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] usbnet: ignore get interface retval of -EINPROGRESS

When calling pm_runtime_get, usb_autopm_get_interface_async
treats a return value of -EINPROGRESS as a success and
increments the usage count.  Since the interface is resuming,
it is safe for usbnet_start_xmit to submit the urb.  If instead,
usbnet_start_xmit treats this as an error the packet will be
dropped.  Additionally, a corresponding tx_complete will not
run to offset the earlier increment of the usage count from the
call to usb_autopm_get_interface_async.

Signed-off-by: James Wylder <james.wylder@...orola.com>
---
 drivers/net/usb/usbnet.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index ce395fe..7dcef05 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -81,6 +81,19 @@

 /*-------------------------------------------------------------------------*/

+/* When power management is configured, usb_autopm_get_interface_async()
+ * will return -EINPROGRESS to signify that the interface was already
+ * resuming at the time that the function was called.  In our case
+ * (all cases?) this not an error condition.
+ * */
+#ifdef CONFIG_PM
+#define USBNET_PM_INTF_ALREADY_RESUMING(x) (x == -EINPROGRESS)
+#else
+#define USBNET_PM_INTF_ALREADY_RESUMING(x) 0
+#endif
+
+/*-------------------------------------------------------------------------*/
+
 // randomly generated ethernet address
 static u8	node_id [ETH_ALEN];

@@ -1042,6 +1055,7 @@ EXPORT_SYMBOL_GPL(usbnet_tx_timeout);

 /*-------------------------------------------------------------------------*/

+
 netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 				     struct net_device *net)
 {
@@ -1105,7 +1119,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,

 	spin_lock_irqsave(&dev->txq.lock, flags);
 	retval = usb_autopm_get_interface_async(dev->intf);
-	if (retval < 0) {
+	if (retval < 0 && !USBNET_PM_INTF_ALREADY_RESUMING(retval)) {
 		spin_unlock_irqrestore(&dev->txq.lock, flags);
 		goto drop;
 	}
-- 
1.7.6

On Sun, Sep 4, 2011 at 5:16 AM, Oliver Neukum <oliver@...kum.org> wrote:
> Am Samstag, 3. September 2011, 19:32:44 schrieb Jim Wylder:
>
> Hi,
>
>> Thanks for the quick feedback.  True usbnet_start_xmit() could be
>> running at anytime, but the usb_autopm_get_interface_async() will only
>> return -EINPROGRESS when rpm_resume detects that
>> dev->power.runtime_status == RPM_RESUMING.  My understanding is that
>> for an asynchronous request, the promise that the device is resuming
>> would be equivalent to cases where usb_autopm_get_interface_async()
>> returns success.
>
> If CONFIG_PM is set.
>
>> In all other cases, when we are not attempting to resume an already
>> resuming interface, this change should have no impact.
>>
>> Are you recommending that I add an additional check for DEV_ASLEEP,
>
> The check is already there but depending on CONFIG_PM.
>
>> possibly to decide whether to drop the or continue on?  Or am I
>> missing your point?  I had not done anything similar because my
>> understanding was that knowing that the device is in fact resuming
>> would be sufficient.
>
> At that point it is sufficient. Upon further thought it looks like your check
> is correct.
> We just need to make very sure that for the decision to queue or
> to submit EVENT_DEV_ASLEEP is relevant.
> Would you consider defining a macro with a nice name for the
> ==0 || == -EINPROGRESS check, so that any reader knows what
> this is about? This is because I doubt only usbnet is affected.
>
>        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