| Message ID | 20131231.162448.1656983197554902144.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
David Miller <davem@davemloft.net> wrote: > vlan: Fix header ops passthru when doing TX VLAN offload. > > When the vlan code detects that the real device can do TX VLAN offloads > in hardware, it tries to arrange for the real device's header_ops to > be invoked directly. Sorry for the late reply. > +static inline int dev_rebuild_header(struct sk_buff *skb) > +{ > + const struct net_device *dev = skb->dev; > + > + if (!dev->header_ops || !dev->header_ops->rebuild) > + return 0; > + return dev->header_ops->rebuild(skb); > +} > + [..] > +static const struct header_ops vlan_passthru_header_ops = { > + .create = vlan_passthru_hard_header, > + .rebuild = dev_rebuild_header, Doesn't that result in infinite recursion when invoking dev_rebuild_header() on skb whose dev->header_ops is vlan_passthru_header_ops? -- 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
From: Florian Westphal <fw@strlen.de> Date: Fri, 3 Jan 2014 12:39:04 +0100 >> +static const struct header_ops vlan_passthru_header_ops = { >> + .create = vlan_passthru_hard_header, >> + .rebuild = dev_rebuild_header, > > Doesn't that result in infinite recursion when invoking > dev_rebuild_header() on skb whose dev->header_ops is > vlan_passthru_header_ops? The skb->dev should be the real_dev at this point, no? -- 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
David Miller <davem@davemloft.net> wrote: > From: Florian Westphal <fw@strlen.de> > Date: Fri, 3 Jan 2014 12:39:04 +0100 > > >> +static const struct header_ops vlan_passthru_header_ops = { > >> + .create = vlan_passthru_hard_header, > >> + .rebuild = dev_rebuild_header, > > > > Doesn't that result in infinite recursion when invoking > > dev_rebuild_header() on skb whose dev->header_ops is > > vlan_passthru_header_ops? > > The skb->dev should be the real_dev at this point, no? Oh, this is fun. I grep'd for invocations of ->rebuild() because I wanted to understand when/where it is used. I only found one single instance, namely neigh_compat_output() in net/core/neighbour.c It does struct net_device *dev = skb->dev; __skb_pull(skb, skb_network_offset(skb)); if (dev_hard_header(skb, dev, ntohs(skb->protocol), NULL, NULL, skb->len) < 0 && dev->header_ops->rebuild(skb)) return 0; So I thought, if skb->dev is the vlan device, we would invoke dev_rebuild_header(), which grabs skb->dev again and invokes dev_rebuild_header again, etc. etc. But: neigh_compat_output (suspicious name...) is only wired up in net/ipv4/arp.c, in 'static const struct neigh_ops arp_broken_ops'. ... and arp_broken_ops is only set if dev->type is one of ARPHRD_ROSE, ARPHRD_AX25, ARPHRD_NETROM. Could it be that ->rebuild() is completely obsolete and could be removed from almost all drivers (except above types)? Archeology exercise #1 digs up 3b04ddde02c in linux.git, which creats header_ops->rebuild, from the old dev->rebuild_header. Exercise #2 then finds commit 275513d2e1c78 in netdev-vger-cvs.git tree. Quote from commit message: - dev->rebuild_header WILL DISAPPEAR. All the code relying on its existance is wrong, though still works. Alexey calling from 1997 ;-) I'll do some more digging before working on this. I've placed a BUG() in eth_rebuild_header on my workstation, lets see if it dies 8-} -- 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
From: Florian Westphal <fw@strlen.de> Date: Sat, 4 Jan 2014 14:51:32 +0100 > Archeology exercise #1 digs up 3b04ddde02c in linux.git, which > creats header_ops->rebuild, from the old dev->rebuild_header. > > Exercise #2 then finds commit 275513d2e1c78 in netdev-vger-cvs.git tree. > Quote from commit message: > > - dev->rebuild_header WILL DISAPPEAR. All the code > relying on its existance is wrong, though still works. > > Alexey calling from 1997 ;-) These protocols which use the broken ARP ops are special, the problem is that they do things like send packets out when their header ops are invoked. It's non-trivial to undo this design, and these aren't easy protocols to test. That's why we've had this compat stuff for them for more than a decade. -- 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
==================== vlan: Fix header ops passthru when doing TX VLAN offload. When the vlan code detects that the real device can do TX VLAN offloads in hardware, it tries to arrange for the real device's header_ops to be invoked directly. But it does so illegally, by simply hooking the real device's header_ops up to the VLAN device. This doesn't work because we will end up invoking a set of header_ops routines which expect a device type which matches the real device, but will see a VLAN device instead. Fix this by providing a pass-thru set of header_ops which will arrange to pass the proper real device instead. To facilitate this add a dev_rebuild_header(). There are implementations which provide a ->cache and ->create but not a ->rebuild (f.e. PLIP). So we need a helper function just like dev_hard_header() to avoid crashes. Use this helper in the one existing place where the header_ops->rebuild was being invoked, the neighbour code. With lots of help from Florian Westphal. Signed-off-by: David S. Miller <davem@davemloft.net> --- include/linux/netdevice.h | 9 +++++++++ net/8021q/vlan_dev.c | 19 ++++++++++++++++++- net/core/neighbour.c | 2 +- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index d9a550b..7514b9c 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1912,6 +1912,15 @@ static inline int dev_parse_header(const struct sk_buff *skb, return dev->header_ops->parse(skb, haddr); } +static inline int dev_rebuild_header(struct sk_buff *skb) +{ + const struct net_device *dev = skb->dev; + + if (!dev->header_ops || !dev->header_ops->rebuild) + return 0; + return dev->header_ops->rebuild(skb); +} + typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr, int len); int register_gifconf(unsigned int family, gifconf_func_t *gifconf); static inline int unregister_gifconf(unsigned int family) diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 762896e..47c908f 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -530,6 +530,23 @@ static const struct header_ops vlan_header_ops = { .parse = eth_header_parse, }; +static int vlan_passthru_hard_header(struct sk_buff *skb, struct net_device *dev, + unsigned short type, + const void *daddr, const void *saddr, + unsigned int len) +{ + struct vlan_dev_priv *vlan = vlan_dev_priv(dev); + struct net_device *real_dev = vlan->real_dev; + + return dev_hard_header(skb, real_dev, type, daddr, saddr, len); +} + +static const struct header_ops vlan_passthru_header_ops = { + .create = vlan_passthru_hard_header, + .rebuild = dev_rebuild_header, + .parse = eth_header_parse, +}; + static struct device_type vlan_type = { .name = "vlan", }; @@ -573,7 +590,7 @@ static int vlan_dev_init(struct net_device *dev) dev->needed_headroom = real_dev->needed_headroom; if (real_dev->features & NETIF_F_HW_VLAN_CTAG_TX) { - dev->header_ops = real_dev->header_ops; + dev->header_ops = &vlan_passthru_header_ops; dev->hard_header_len = real_dev->hard_header_len; } else { dev->header_ops = &vlan_header_ops; diff --git a/net/core/neighbour.c b/net/core/neighbour.c index 36b1443..932c6d7 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -1275,7 +1275,7 @@ int neigh_compat_output(struct neighbour *neigh, struct sk_buff *skb) if (dev_hard_header(skb, dev, ntohs(skb->protocol), NULL, NULL, skb->len) < 0 && - dev->header_ops->rebuild(skb)) + dev_rebuild_header(skb)) return 0; return dev_queue_xmit(skb);