* [PATCH] net: Fix skb_under_panic oops in neigh_resolve_output
@ 2012-10-05 3:05 ramesh.nagappa
2012-10-05 6:27 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: ramesh.nagappa @ 2012-10-05 3:05 UTC (permalink / raw)
To: bnramesh
Cc: davem, edumazet, ebiederm, xemul, netdev, linux-kernel, Ramesh Nagappa
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));
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));
--
1.7.11.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net: Fix skb_under_panic oops in neigh_resolve_output
2012-10-05 3:05 [PATCH] net: Fix skb_under_panic oops in neigh_resolve_output ramesh.nagappa
@ 2012-10-05 6:27 ` Eric Dumazet
2012-10-05 16:33 ` Ramesh Nagappa
0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2012-10-05 6:27 UTC (permalink / raw)
To: ramesh.nagappa
Cc: bnramesh, davem, edumazet, ebiederm, xemul, netdev, linux-kernel,
Ramesh Nagappa
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] net: Fix skb_under_panic oops in neigh_resolve_output
2012-10-05 6:27 ` Eric Dumazet
@ 2012-10-05 16:33 ` Ramesh Nagappa
2012-10-05 16:59 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: Ramesh Nagappa @ 2012-10-05 16:33 UTC (permalink / raw)
To: Eric Dumazet, ramesh.nagappa
Cc: davem, edumazet, ebiederm, xemul, netdev, linux-kernel
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] net: Fix skb_under_panic oops in neigh_resolve_output
2012-10-05 16:33 ` Ramesh Nagappa
@ 2012-10-05 16:59 ` Eric Dumazet
0 siblings, 0 replies; 4+ messages in thread
From: Eric Dumazet @ 2012-10-05 16:59 UTC (permalink / raw)
To: Ramesh Nagappa
Cc: ramesh.nagappa, davem, edumazet, ebiederm, xemul, netdev, linux-kernel
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()
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-10-05 16:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-05 3:05 [PATCH] net: Fix skb_under_panic oops in neigh_resolve_output ramesh.nagappa
2012-10-05 6:27 ` Eric Dumazet
2012-10-05 16:33 ` Ramesh Nagappa
2012-10-05 16:59 ` Eric Dumazet
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.