Message ID | 1349406325-3157-1-git-send-email-ramesh.nagappa@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, 2012-10-04 at 20:05 -0700, ramesh.nagappa@gmail.com wrote: > From: Ramesh Nagappa <ramesh.nagappa@ericsson.com> > > The retry loop in neigh_resolve_output() and neigh_connected_output() > call dev_hard_header() with out reseting the skb to network_header. > This causes the retry to fail with skb_under_panic. The fix is to > reset the network_header within the retry loop. > > Signed-off-by: Ramesh Nagappa <ramesh.nagappa@ericsson.com> > Reviewed-by: Shawn Lu <shawn.lu@ericsson.com> > Reviewed-by: Robert Coulson <robert.coulson@ericsson.com> > Reviewed-by: Billie Alsup <billie.alsup@ericsson.com> > --- > net/core/neighbour.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index 117afaf..f50c542 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -1313,6 +1313,7 @@ int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb) > > do { > seq = read_seqbegin(&neigh->ha_lock); > + __skb_pull(skb, skb_network_offset(skb)); This __skb_pull() could be done before the read_seqbegin() to keep the protected section short. > err = dev_hard_header(skb, dev, ntohs(skb->protocol), > neigh->ha, NULL, skb->len); > } while (read_seqretry(&neigh->ha_lock, seq)); > @@ -1342,10 +1343,9 @@ int neigh_connected_output(struct neighbour *neigh, struct sk_buff *skb) > unsigned int seq; > int err; > > - __skb_pull(skb, skb_network_offset(skb)); > - > do { > seq = read_seqbegin(&neigh->ha_lock); > + __skb_pull(skb, skb_network_offset(skb)); This __skb_pull() could be done before the read_seqbegin() to keep the protected section short. > err = dev_hard_header(skb, dev, ntohs(skb->protocol), > neigh->ha, NULL, skb->len); > } while (read_seqretry(&neigh->ha_lock, seq)); Thanks -- 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
Hi Eric, Yes, that is a good optimization. neigh_resolve_output() also has the __skb_pull() outside the loop, is that required ? The changes would be like ... neigh_resolve_output() ... - __skb_pull(skb, skb_network_offset(skb)); if (!neigh_event_send(neigh, skb)) { int err; struct net_device *dev = neigh->dev; unsigned int seq; if (dev->header_ops->cache && !neigh->hh.hh_len) neigh_hh_init(neigh, dst); do { + __skb_pull(skb, skb_network_offset(skb)); seq = read_seqbegin(&neigh->ha_lock); err = dev_hard_header(skb, dev, ntohs(skb->protocol), neigh->ha, NULL, skb->len); } while (read_seqretry(&neigh->ha_lock, seq)); Thanks, Ramesh > -----Original Message----- > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > do { > > seq = read_seqbegin(&neigh->ha_lock); > > + __skb_pull(skb, skb_network_offset(skb)); > > This __skb_pull() could be done before the read_seqbegin() to > keep the protected section short. > > > err = dev_hard_header(skb, dev, ntohs(skb->protocol), > > neigh->ha, NULL, skb->len); > > } while (read_seqretry(&neigh->ha_lock, seq)); > > Thanks > > > -- 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
On Fri, 2012-10-05 at 12:33 -0400, Ramesh Nagappa wrote: > Hi Eric, > > Yes, that is a good optimization. neigh_resolve_output() also has the > __skb_pull() outside the loop, is that required ? The changes would be > like ... > > neigh_resolve_output() > ... > - __skb_pull(skb, skb_network_offset(skb)); > > > if (!neigh_event_send(neigh, skb)) { > int err; > struct net_device *dev = neigh->dev; > unsigned int seq; > > if (dev->header_ops->cache && !neigh->hh.hh_len) > neigh_hh_init(neigh, dst); > > do { > + __skb_pull(skb, skb_network_offset(skb)); > seq = read_seqbegin(&neigh->ha_lock); > err = dev_hard_header(skb, dev, ntohs(skb->protocol), > neigh->ha, NULL, skb->len); > } while (read_seqretry(&neigh->ha_lock, seq)); All similar constructions should be audited. For example in net/decnet/dn_neigh.c , dn_neigh_output_packet() -- 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
diff --git a/net/core/neighbour.c b/net/core/neighbour.c index 117afaf..f50c542 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -1313,6 +1313,7 @@ int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb) do { seq = read_seqbegin(&neigh->ha_lock); + __skb_pull(skb, skb_network_offset(skb)); err = dev_hard_header(skb, dev, ntohs(skb->protocol), neigh->ha, NULL, skb->len); } while (read_seqretry(&neigh->ha_lock, seq)); @@ -1342,10 +1343,9 @@ int neigh_connected_output(struct neighbour *neigh, struct sk_buff *skb) unsigned int seq; int err; - __skb_pull(skb, skb_network_offset(skb)); - do { seq = read_seqbegin(&neigh->ha_lock); + __skb_pull(skb, skb_network_offset(skb)); err = dev_hard_header(skb, dev, ntohs(skb->protocol), neigh->ha, NULL, skb->len); } while (read_seqretry(&neigh->ha_lock, seq));