* [PATCH] Add missing skb->dev assignment in Frame Relay RX code @ 2008-06-29 19:48 Krzysztof Halasa 2008-06-29 20:04 ` Stephen Hemminger 2008-07-04 12:13 ` Jeff Garzik 0 siblings, 2 replies; 7+ messages in thread From: Krzysztof Halasa @ 2008-06-29 19:48 UTC (permalink / raw) To: Jeff Garzik, Andrew Morton; +Cc: netdev, lkml Hi, Commit 4c13eb6657fe9ef7b4dc8f1a405c902e9e5234e0 ([ETH]: Make eth_type_trans set skb->dev like the other *_type_trans) removed skb->dev assignment from hdlc_fr.c:fr_rx(). Unfortunately it was also needed for cases other than eth_type_trans(). Adding it back. It's quite serious and may be a security risk as it causes a wrong input interface indication (the physical hdlcX instead of logical pvcX). Probably -stable class fix. Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c index c4ab032..3a86e64 100644 --- a/drivers/net/wan/hdlc_fr.c +++ b/drivers/net/wan/hdlc_fr.c @@ -1008,6 +1008,7 @@ static int fr_rx(struct sk_buff *skb) stats->rx_bytes += skb->len; if (pvc->state.becn) stats->rx_compressed++; + skb->dev = dev; netif_rx(skb); return NET_RX_SUCCESS; } else { ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Add missing skb->dev assignment in Frame Relay RX code 2008-06-29 19:48 [PATCH] Add missing skb->dev assignment in Frame Relay RX code Krzysztof Halasa @ 2008-06-29 20:04 ` Stephen Hemminger 2008-06-29 21:10 ` Krzysztof Halasa 2008-07-04 12:13 ` Jeff Garzik 1 sibling, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2008-06-29 20:04 UTC (permalink / raw) To: Krzysztof Halasa; +Cc: Jeff Garzik, Andrew Morton, netdev, lkml On Sun, 29 Jun 2008 21:48:11 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote: > Hi, > > Commit 4c13eb6657fe9ef7b4dc8f1a405c902e9e5234e0 ([ETH]: Make > eth_type_trans set skb->dev like the other *_type_trans) removed > skb->dev assignment from hdlc_fr.c:fr_rx(). Unfortunately it was also > needed for cases other than eth_type_trans(). > > Adding it back. > > It's quite serious and may be a security risk as it causes a wrong > input interface indication (the physical hdlcX instead of logical > pvcX). Probably -stable class fix. > > Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl> > > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > index c4ab032..3a86e64 100644 > --- a/drivers/net/wan/hdlc_fr.c > +++ b/drivers/net/wan/hdlc_fr.c > @@ -1008,6 +1008,7 @@ static int fr_rx(struct sk_buff *skb) > stats->rx_bytes += skb->len; > if (pvc->state.becn) > stats->rx_compressed++; > + skb->dev = dev; > netif_rx(skb); > return NET_RX_SUCCESS; > } else { > -- > 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 Better to use netdev_alloc_skb for receive buffers instead. --- a/drivers/net/wan/hdlc_fr.c 2008-06-29 13:02:42.000000000 -0700 +++ b/drivers/net/wan/hdlc_fr.c 2008-06-29 13:04:01.000000000 -0700 @@ -515,7 +515,7 @@ static void fr_lmi_send(struct net_devic } } - skb = dev_alloc_skb(len); + skb = netdev_alloc_skb(dev, len); if (!skb) { printk(KERN_WARNING "%s: Memory squeeze on fr_lmi_send()\n", dev->name); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add missing skb->dev assignment in Frame Relay RX code 2008-06-29 20:04 ` Stephen Hemminger @ 2008-06-29 21:10 ` Krzysztof Halasa 2008-06-30 17:19 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: Krzysztof Halasa @ 2008-06-29 21:10 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Jeff Garzik, Andrew Morton, netdev, lkml Stephen Hemminger <shemminger@vyatta.com> writes: >> --- a/drivers/net/wan/hdlc_fr.c >> +++ b/drivers/net/wan/hdlc_fr.c >> @@ -1008,6 +1008,7 @@ static int fr_rx(struct sk_buff *skb) >> stats->rx_bytes += skb->len; >> if (pvc->state.becn) >> stats->rx_compressed++; >> + skb->dev = dev; >> netif_rx(skb); >> return NET_RX_SUCCESS; >> } else { > > Better to use netdev_alloc_skb for receive buffers instead. > --- a/drivers/net/wan/hdlc_fr.c 2008-06-29 13:02:42.000000000 -0700 > +++ b/drivers/net/wan/hdlc_fr.c 2008-06-29 13:04:01.000000000 -0700 > @@ -515,7 +515,7 @@ static void fr_lmi_send(struct net_devic > } > } > > - skb = dev_alloc_skb(len); > + skb = netdev_alloc_skb(dev, len); > if (!skb) { > printk(KERN_WARNING "%s: Memory squeeze on fr_lmi_send()\n", > dev->name); Well, no, that's another story - the missing assignment is in fr_rx(), in the regular receive path (similar to 802.1q case, ethX -> ethX.VLAN_ID transition). I.e., we get incoming packet from a hardware driver, with skb->dev pointing to hardware hdlcX device, and we change it to point to a logical pvcX device. fr_lmi_send() is for control messages (generated locally) only, in TX path. IOW we alloc skb and send it through hw driver immediately. Not related to the bug. That said, perhaps I should indeed use netdev_alloc_skb() for those control messages? Or is it RX-only? -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add missing skb->dev assignment in Frame Relay RX code 2008-06-29 21:10 ` Krzysztof Halasa @ 2008-06-30 17:19 ` Stephen Hemminger 2008-06-30 18:12 ` Krzysztof Halasa 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2008-06-30 17:19 UTC (permalink / raw) To: Krzysztof Halasa; +Cc: Jeff Garzik, Andrew Morton, netdev, lkml On Sun, 29 Jun 2008 23:10:11 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote: > Stephen Hemminger <shemminger@vyatta.com> writes: > > >> --- a/drivers/net/wan/hdlc_fr.c > >> +++ b/drivers/net/wan/hdlc_fr.c > >> @@ -1008,6 +1008,7 @@ static int fr_rx(struct sk_buff *skb) > >> stats->rx_bytes += skb->len; > >> if (pvc->state.becn) > >> stats->rx_compressed++; > >> + skb->dev = dev; > >> netif_rx(skb); > >> return NET_RX_SUCCESS; > >> } else { > > > > Better to use netdev_alloc_skb for receive buffers instead. > > --- a/drivers/net/wan/hdlc_fr.c 2008-06-29 13:02:42.000000000 -0700 > > +++ b/drivers/net/wan/hdlc_fr.c 2008-06-29 13:04:01.000000000 -0700 > > @@ -515,7 +515,7 @@ static void fr_lmi_send(struct net_devic > > } > > } > > > > - skb = dev_alloc_skb(len); > > + skb = netdev_alloc_skb(dev, len); > > if (!skb) { > > printk(KERN_WARNING "%s: Memory squeeze on fr_lmi_send()\n", > > dev->name); > > Well, no, that's another story - the missing assignment is in fr_rx(), > in the regular receive path (similar to 802.1q case, ethX -> > ethX.VLAN_ID transition). I.e., we get incoming packet from a hardware > driver, with skb->dev pointing to hardware hdlcX device, and we change > it to point to a logical pvcX device. > > fr_lmi_send() is for control messages (generated locally) only, in TX > path. IOW we alloc skb and send it through hw driver immediately. Not > related to the bug. > > That said, perhaps I should indeed use netdev_alloc_skb() for those > control messages? Or is it RX-only? netdev_alloc_skb does two things: 1) it sets skb->dev 2) on some platforms it can choose memory "closer" to the device. but this is really a NUMA issue ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add missing skb->dev assignment in Frame Relay RX code 2008-06-30 17:19 ` Stephen Hemminger @ 2008-06-30 18:12 ` Krzysztof Halasa 0 siblings, 0 replies; 7+ messages in thread From: Krzysztof Halasa @ 2008-06-30 18:12 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Jeff Garzik, Andrew Morton, netdev, lkml Stephen Hemminger <shemminger@vyatta.com> writes: > netdev_alloc_skb does two things: > 1) it sets skb->dev > 2) on some platforms it can choose memory "closer" to the device. > but this is really a NUMA issue I see. It looks like I should use it for sending control messages then. Thanks. -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add missing skb->dev assignment in Frame Relay RX code 2008-06-29 19:48 [PATCH] Add missing skb->dev assignment in Frame Relay RX code Krzysztof Halasa 2008-06-29 20:04 ` Stephen Hemminger @ 2008-07-04 12:13 ` Jeff Garzik 2008-07-04 12:39 ` Krzysztof Halasa 1 sibling, 1 reply; 7+ messages in thread From: Jeff Garzik @ 2008-07-04 12:13 UTC (permalink / raw) To: Krzysztof Halasa; +Cc: Andrew Morton, netdev, lkml Krzysztof Halasa wrote: > Hi, > > Commit 4c13eb6657fe9ef7b4dc8f1a405c902e9e5234e0 ([ETH]: Make > eth_type_trans set skb->dev like the other *_type_trans) removed > skb->dev assignment from hdlc_fr.c:fr_rx(). Unfortunately it was also > needed for cases other than eth_type_trans(). > > Adding it back. > > It's quite serious and may be a security risk as it causes a wrong > input interface indication (the physical hdlcX instead of logical > pvcX). Probably -stable class fix. > > Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl> > > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > index c4ab032..3a86e64 100644 > --- a/drivers/net/wan/hdlc_fr.c > +++ b/drivers/net/wan/hdlc_fr.c > @@ -1008,6 +1008,7 @@ static int fr_rx(struct sk_buff *skb) > stats->rx_bytes += skb->len; > if (pvc->state.becn) > stats->rx_compressed++; > + skb->dev = dev; > netif_rx(skb); > return NET_RX_SUCCESS; > } else { applied ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add missing skb->dev assignment in Frame Relay RX code 2008-07-04 12:13 ` Jeff Garzik @ 2008-07-04 12:39 ` Krzysztof Halasa 0 siblings, 0 replies; 7+ messages in thread From: Krzysztof Halasa @ 2008-07-04 12:39 UTC (permalink / raw) To: Jeff Garzik; +Cc: Andrew Morton, netdev, lkml Jeff Garzik <jeff@garzik.org> writes: >> Commit 4c13eb6657fe9ef7b4dc8f1a405c902e9e5234e0 ([ETH]: Make >> eth_type_trans set skb->dev like the other *_type_trans) removed >> skb->dev assignment from hdlc_fr.c:fr_rx(). Unfortunately it was also >> needed for cases other than eth_type_trans(). >> >> Adding it back. > applied Thanks. -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-07-04 12:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-06-29 19:48 [PATCH] Add missing skb->dev assignment in Frame Relay RX code Krzysztof Halasa 2008-06-29 20:04 ` Stephen Hemminger 2008-06-29 21:10 ` Krzysztof Halasa 2008-06-30 17:19 ` Stephen Hemminger 2008-06-30 18:12 ` Krzysztof Halasa 2008-07-04 12:13 ` Jeff Garzik 2008-07-04 12:39 ` Krzysztof Halasa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).