* Very Minor Patch to loopback.c
@ 2014-06-12 19:53 Zachary
2014-06-12 20:24 ` Zachary
2014-06-12 20:27 ` Florian Fainelli
0 siblings, 2 replies; 6+ messages in thread
From: Zachary @ 2014-06-12 19:53 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller
Very minor patch to loopback.c that I noticed while comparing the
dummy.c code to loopback.c code (dummy.c was based on loopback.c). I
did not see a maintainer in MAINTAINERS in my cursory glance, so I
apologize if this is to the wrong place. I am testing this patch
right now on my laptop and it appears to be working without issues.
It also compiled without warnings.
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index bb96409..f9f88d7 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -22,6 +22,7 @@
* interface.
* Alexey Kuznetsov: Potential hang under some extreme
* cases removed.
+ * Zachary Winnerman: Extremely minor patch in loopback_xmit
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -72,7 +73,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct pcpu_lstats *lb_stats;
- int len;
skb_orphan(skb);
@@ -86,10 +86,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
/* it's OK to use per_cpu_ptr() because BHs are off */
lb_stats = this_cpu_ptr(dev->lstats);
- len = skb->len;
if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
u64_stats_update_begin(&lb_
stats->syncp);
- lb_stats->bytes += len;
+ lb_stats->bytes += skb->len;
lb_stats->packets++;
u64_stats_update_end(&lb_stats->syncp);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Very Minor Patch to loopback.c
2014-06-12 19:53 Very Minor Patch to loopback.c Zachary
@ 2014-06-12 20:24 ` Zachary
2014-06-12 22:22 ` David Miller
2014-06-12 20:27 ` Florian Fainelli
1 sibling, 1 reply; 6+ messages in thread
From: Zachary @ 2014-06-12 20:24 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller
Updated due to Richard Weinberger's info in linux-kernel
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index bb96409..fc03883 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -72,7 +72,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct pcpu_lstats *lb_stats;
- int len;
skb_orphan(skb);
@@ -86,10 +85,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
/* it's OK to use per_cpu_ptr() because BHs are off */
lb_stats = this_cpu_ptr(dev->lstats);
- len = skb->len;
if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
u64_stats_update_begin(&lb_stats->syncp);
- lb_stats->bytes += len;
+ lb_stats->bytes += skb->len;
lb_stats->packets++;
u64_stats_update_end(&lb_stats->syncp);
}
On Thu, Jun 12, 2014 at 3:53 PM, Zachary <zacharyw09264@gmail.com> wrote:
> Very minor patch to loopback.c that I noticed while comparing the
> dummy.c code to loopback.c code (dummy.c was based on loopback.c). I
> did not see a maintainer in MAINTAINERS in my cursory glance, so I
> apologize if this is to the wrong place. I am testing this patch
> right now on my laptop and it appears to be working without issues.
> It also compiled without warnings.
>
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index bb96409..f9f88d7 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -22,6 +22,7 @@
> * interface.
> * Alexey Kuznetsov: Potential hang under some extreme
> * cases removed.
> + * Zachary Winnerman: Extremely minor patch in loopback_xmit
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -72,7 +73,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> struct pcpu_lstats *lb_stats;
> - int len;
>
> skb_orphan(skb);
>
> @@ -86,10 +86,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> /* it's OK to use per_cpu_ptr() because BHs are off */
> lb_stats = this_cpu_ptr(dev->lstats);
>
> - len = skb->len;
> if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
> u64_stats_update_begin(&lb_
> stats->syncp);
> - lb_stats->bytes += len;
> + lb_stats->bytes += skb->len;
> lb_stats->packets++;
> u64_stats_update_end(&lb_stats->syncp);
> }
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Very Minor Patch to loopback.c
2014-06-12 19:53 Very Minor Patch to loopback.c Zachary
2014-06-12 20:24 ` Zachary
@ 2014-06-12 20:27 ` Florian Fainelli
2014-06-12 20:30 ` Zachary
1 sibling, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2014-06-12 20:27 UTC (permalink / raw)
To: Zachary; +Cc: netdev, David S. Miller
2014-06-12 12:53 GMT-07:00 Zachary <zacharyw09264@gmail.com>:
> Very minor patch to loopback.c that I noticed while comparing the
> dummy.c code to loopback.c code (dummy.c was based on loopback.c). I
> did not see a maintainer in MAINTAINERS in my cursory glance, so I
> apologize if this is to the wrong place. I am testing this patch
> right now on my laptop and it appears to be working without issues.
> It also compiled without warnings.
Your patch introduces a potential use after free type of bug. As soon
as the skb pointer has been passed to netif_rx(), the network stack is
allowed to free this skb as soon as it has determined it is unused,
this is the reason why this 'len' variable exists in the first place.
>
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index bb96409..f9f88d7 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -22,6 +22,7 @@
> * interface.
> * Alexey Kuznetsov: Potential hang under some extreme
> * cases removed.
> + * Zachary Winnerman: Extremely minor patch in loopback_xmit
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -72,7 +73,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> struct pcpu_lstats *lb_stats;
> - int len;
>
> skb_orphan(skb);
>
> @@ -86,10 +86,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> /* it's OK to use per_cpu_ptr() because BHs are off */
> lb_stats = this_cpu_ptr(dev->lstats);
>
> - len = skb->len;
> if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
> u64_stats_update_begin(&lb_
> stats->syncp);
> - lb_stats->bytes += len;
> + lb_stats->bytes += skb->len;
> lb_stats->packets++;
> u64_stats_update_end(&lb_stats->syncp);
> }
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Very Minor Patch to loopback.c
2014-06-12 20:27 ` Florian Fainelli
@ 2014-06-12 20:30 ` Zachary
2014-06-12 20:46 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Zachary @ 2014-06-12 20:30 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, David S. Miller
Should this be changed in dummy.c then to prevent the same bug?
On Thu, Jun 12, 2014 at 4:27 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 2014-06-12 12:53 GMT-07:00 Zachary <zacharyw09264@gmail.com>:
>> Very minor patch to loopback.c that I noticed while comparing the
>> dummy.c code to loopback.c code (dummy.c was based on loopback.c). I
>> did not see a maintainer in MAINTAINERS in my cursory glance, so I
>> apologize if this is to the wrong place. I am testing this patch
>> right now on my laptop and it appears to be working without issues.
>> It also compiled without warnings.
>
> Your patch introduces a potential use after free type of bug. As soon
> as the skb pointer has been passed to netif_rx(), the network stack is
> allowed to free this skb as soon as it has determined it is unused,
> this is the reason why this 'len' variable exists in the first place.
>
>>
>> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
>> index bb96409..f9f88d7 100644
>> --- a/drivers/net/loopback.c
>> +++ b/drivers/net/loopback.c
>> @@ -22,6 +22,7 @@
>> * interface.
>> * Alexey Kuznetsov: Potential hang under some extreme
>> * cases removed.
>> + * Zachary Winnerman: Extremely minor patch in loopback_xmit
>> *
>> * This program is free software; you can redistribute it and/or
>> * modify it under the terms of the GNU General Public License
>> @@ -72,7 +73,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>> struct net_device *dev)
>> {
>> struct pcpu_lstats *lb_stats;
>> - int len;
>>
>> skb_orphan(skb);
>>
>> @@ -86,10 +86,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>> /* it's OK to use per_cpu_ptr() because BHs are off */
>> lb_stats = this_cpu_ptr(dev->lstats);
>>
>> - len = skb->len;
>> if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
>> u64_stats_update_begin(&lb_
>> stats->syncp);
>> - lb_stats->bytes += len;
>> + lb_stats->bytes += skb->len;
>> lb_stats->packets++;
>> u64_stats_update_end(&lb_stats->syncp);
>> }
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Very Minor Patch to loopback.c
2014-06-12 20:30 ` Zachary
@ 2014-06-12 20:46 ` Florian Fainelli
0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2014-06-12 20:46 UTC (permalink / raw)
To: Zachary; +Cc: netdev, David S. Miller
2014-06-12 13:30 GMT-07:00 Zachary <zacharyw09264@gmail.com>:
> Should this be changed in dummy.c then to prevent the same bug?
dummy.c uses skb->len before calling dev_kfree_skb(), and in its
transmit path, which is a valid use case that will not introduce use
after free issues.
(BTW: we avoid top-posting on netdev and other linux mailing-lists)
>
> On Thu, Jun 12, 2014 at 4:27 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> 2014-06-12 12:53 GMT-07:00 Zachary <zacharyw09264@gmail.com>:
>>> Very minor patch to loopback.c that I noticed while comparing the
>>> dummy.c code to loopback.c code (dummy.c was based on loopback.c). I
>>> did not see a maintainer in MAINTAINERS in my cursory glance, so I
>>> apologize if this is to the wrong place. I am testing this patch
>>> right now on my laptop and it appears to be working without issues.
>>> It also compiled without warnings.
>>
>> Your patch introduces a potential use after free type of bug. As soon
>> as the skb pointer has been passed to netif_rx(), the network stack is
>> allowed to free this skb as soon as it has determined it is unused,
>> this is the reason why this 'len' variable exists in the first place.
>>
>>>
>>> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
>>> index bb96409..f9f88d7 100644
>>> --- a/drivers/net/loopback.c
>>> +++ b/drivers/net/loopback.c
>>> @@ -22,6 +22,7 @@
>>> * interface.
>>> * Alexey Kuznetsov: Potential hang under some extreme
>>> * cases removed.
>>> + * Zachary Winnerman: Extremely minor patch in loopback_xmit
>>> *
>>> * This program is free software; you can redistribute it and/or
>>> * modify it under the terms of the GNU General Public License
>>> @@ -72,7 +73,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>>> struct net_device *dev)
>>> {
>>> struct pcpu_lstats *lb_stats;
>>> - int len;
>>>
>>> skb_orphan(skb);
>>>
>>> @@ -86,10 +86,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>>> /* it's OK to use per_cpu_ptr() because BHs are off */
>>> lb_stats = this_cpu_ptr(dev->lstats);
>>>
>>> - len = skb->len;
>>> if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
>>> u64_stats_update_begin(&lb_
>>> stats->syncp);
>>> - lb_stats->bytes += len;
>>> + lb_stats->bytes += skb->len;
>>> lb_stats->packets++;
>>> u64_stats_update_end(&lb_stats->syncp);
>>> }
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Florian
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Very Minor Patch to loopback.c
2014-06-12 20:24 ` Zachary
@ 2014-06-12 22:22 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2014-06-12 22:22 UTC (permalink / raw)
To: zacharyw09264; +Cc: netdev
From: Zachary <zacharyw09264@gmail.com>
Date: Thu, 12 Jun 2014 16:24:03 -0400
> Updated due to Richard Weinberger's info in linux-kernel
>
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index bb96409..fc03883 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -72,7 +72,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> struct pcpu_lstats *lb_stats;
> - int len;
>
> skb_orphan(skb);
>
> @@ -86,10 +85,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> /* it's OK to use per_cpu_ptr() because BHs are off */
> lb_stats = this_cpu_ptr(dev->lstats);
>
> - len = skb->len;
> if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
> u64_stats_update_begin(&lb_stats->syncp);
> - lb_stats->bytes += len;
> + lb_stats->bytes += skb->len;
> lb_stats->packets++;
> u64_stats_update_end(&lb_stats->syncp);
> }
After 'skb' is passed to netif_rx(), it can no longer be accessed because
that function can free 'skb'.
That is why we load skb->len into a local variable, before calling
netif_rx().
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-12 22:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-12 19:53 Very Minor Patch to loopback.c Zachary
2014-06-12 20:24 ` Zachary
2014-06-12 22:22 ` David Miller
2014-06-12 20:27 ` Florian Fainelli
2014-06-12 20:30 ` Zachary
2014-06-12 20:46 ` Florian Fainelli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.