[PATCH RFC] net: neighbour: use source address of last enqueued packet for solicitation

[PATCH RFC] net: neighbour: use source address of last enqueued packet for solicitation

[Hannes Frederic Sowa]

Currently we always use the first member of the arp_queue to determine
the sender ip address of the arp packet (or in case of IPv6 - source
address of the ndisc packet). This skb is fixed as long as the queue is
not drained by a complete purge because of a timeout or by a successful
response.

If the first packet enqueued on the arp_queue is from a local application
with a manually set source address and the to be discovered system
does some kind of uRPF checks on the source address in the arp packet
the resolving process hangs until a timeout and restarts. This hurts
communication with the participating network node.

This could be mitigated a bit if we use the latest enqueued skb's
source address for the resolving process, which is not as static as
the arp_queue's head. This change of the source address could result in
better recovery of a failed solicitation.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---

I didn't find anything which could break because of this change, but
please have a second look.

 net/core/neighbour.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 6072610..ca15f32 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -867,7 +867,7 @@ static void neigh_invalidate(struct neighbour *neigh)
 static void neigh_probe(struct neighbour *neigh)
 	__releases(neigh->lock)
 {
-	struct sk_buff *skb = skb_peek(&neigh->arp_queue);
+	struct sk_buff *skb = skb_peek_tail(&neigh->arp_queue);
 	/* keep skb alive even if arp_queue overflows */
 	if (skb)
 		skb = skb_copy(skb, GFP_ATOMIC);
-- 
1.8.3.1

Re: [PATCH RFC] net: neighbour: use source address of last enqueued packet for solicitation

[Julian Anastasov]

	Hello,

On Sun, 8 Sep 2013, Hannes Frederic Sowa wrote:

> Currently we always use the first member of the arp_queue to determine
> the sender ip address of the arp packet (or in case of IPv6 - source
> address of the ndisc packet). This skb is fixed as long as the queue is
> not drained by a complete purge because of a timeout or by a successful
> response.
> 
> If the first packet enqueued on the arp_queue is from a local application
> with a manually set source address and the to be discovered system
> does some kind of uRPF checks on the source address in the arp packet
> the resolving process hangs until a timeout and restarts. This hurts
> communication with the participating network node.
> 
> This could be mitigated a bit if we use the latest enqueued skb's
> source address for the resolving process, which is not as static as
> the arp_queue's head. This change of the source address could result in
> better recovery of a failed solicitation.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
> 
> I didn't find anything which could break because of this change, but
> please have a second look.

	arp_queue has packets only in NUD_INCOMPLETE state 
(mcast_solicit=3 secs by default). And __neigh_event_send()
now can keep many packets, 64KB from recent changes. So the
1st place is not guaranteed but now it is more difficult
to kick the first packet compared to the old limit of just
3 packets.

	The change can give chance for 2nd and 3th
probe if the 1st probe is not replied, so it should be
better to apply it:

Reviewed-by: Julian Anastasov <ja@ssi.bg>

	Still, I think such problems should be addressed
with conf/{DEV,all}/arp_announce=1 or 2.

>  net/core/neighbour.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 6072610..ca15f32 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -867,7 +867,7 @@ static void neigh_invalidate(struct neighbour *neigh)
>  static void neigh_probe(struct neighbour *neigh)
>  	__releases(neigh->lock)
>  {
> -	struct sk_buff *skb = skb_peek(&neigh->arp_queue);
> +	struct sk_buff *skb = skb_peek_tail(&neigh->arp_queue);
>  	/* keep skb alive even if arp_queue overflows */
>  	if (skb)
>  		skb = skb_copy(skb, GFP_ATOMIC);
> -- 
> 1.8.3.1

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH RFC] net: neighbour: use source address of last enqueued packet for solicitation

[Hannes Frederic Sowa]

On Mon, Sep 09, 2013 at 11:17:45PM +0300, Julian Anastasov wrote:
> 	arp_queue has packets only in NUD_INCOMPLETE state 
> (mcast_solicit=3 secs by default). And __neigh_event_send()
> now can keep many packets, 64KB from recent changes. So the
> 1st place is not guaranteed but now it is more difficult
> to kick the first packet compared to the old limit of just
> 3 packets.
> 
> 	The change can give chance for 2nd and 3th
> probe if the 1st probe is not replied, so it should be
> better to apply it:
> 
> Reviewed-by: Julian Anastasov <ja@ssi.bg>

Thanks for the review. I resend the patch as soon as net-next opens.

> 
> 	Still, I think such problems should be addressed
> with conf/{DEV,all}/arp_announce=1 or 2.

*nod*

I use this knob if I have such problems. But this patch improves
connectivity in the default configuration and we actually don't care much
about the source address in either ipv4 or ipv6. So it seemed legitimate
and simple to me.

Greetings,

  Hannes