All of lore.kernel.org
 help / color / mirror / Atom feed
* [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message
@ 2022-02-07 14:09 wanghai (M)
  2022-02-08  7:51 ` Ido Schimmel
  0 siblings, 1 reply; 6+ messages in thread
From: wanghai (M) @ 2022-02-07 14:09 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, edumazet, yoshfuji, dsahern
  Cc: netdev, linux-kernel

Hello,

I found a bug, but I don't know how to fix it. Anyone have some good ideas?

This bug will cause udp broadcast messages not to be sent, but instead send
non-expected arp request messages.

Deleting the ip while sending udp broadcast messages and then configuring
the ip again will probably trigger the bug.

The following is the timing diagram of the bug, cpu0 sends a broadcast
message and cpu1 deletes the routing table at the appropriate time.

cpu0                                        cpu1
send()
   udp_sendmsg()
     ip_route_output_flow()
     |  fib_lookup()
     udp_send_skb()
       ip_send_skb()
         ip_finish_output2()

                                             ifconfig eth0:2 down
                                               fib_del_ifaddr
                                                 fib_table_delete // 
delete fib table

           ip_neigh_for_gw()
           |  ip_neigh_gw4()
           |    __ipv4_neigh_lookup_noref()
           |    __neigh_create()
           |      tbl->constructor(n) --> arp_constructor()
           |        neigh->type = inet_addr_type_dev_table(); // no 
route, neigh->type = RTN_UNICAST
           neigh_output() // unicast, send an arp request and create an 
exception arp entry

After the above operation, an abnormal arp entry will be generated. If
the ip is configured again(ifconfig eth0:2 12.0.208.0), the abnormal arp
entry will still exist, and the udp broadcast message will be converted
to an arp request message when it is sent.

Any feedback would be appreciated, thanks.

-- 
Wang Hai


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message
  2022-02-07 14:09 [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message wanghai (M)
@ 2022-02-08  7:51 ` Ido Schimmel
  2022-02-08  9:18   ` wanghai (M)
  2022-02-16  3:19   ` wanghai (M)
  0 siblings, 2 replies; 6+ messages in thread
From: Ido Schimmel @ 2022-02-08  7:51 UTC (permalink / raw)
  To: wanghai (M)
  Cc: David Miller, Jakub Kicinski, edumazet, yoshfuji, dsahern,
	netdev, linux-kernel

On Mon, Feb 07, 2022 at 10:09:49PM +0800, wanghai (M) wrote:
> Hello,
> 
> I found a bug, but I don't know how to fix it. Anyone have some good ideas?
> 
> This bug will cause udp broadcast messages not to be sent, but instead send
> non-expected arp request messages.
> 
> Deleting the ip while sending udp broadcast messages and then configuring
> the ip again will probably trigger the bug.
> 
> The following is the timing diagram of the bug, cpu0 sends a broadcast
> message and cpu1 deletes the routing table at the appropriate time.
> 
> cpu0                                        cpu1
> send()
>   udp_sendmsg()
>     ip_route_output_flow()
>     |  fib_lookup()
>     udp_send_skb()
>       ip_send_skb()
>         ip_finish_output2()
> 
>                                             ifconfig eth0:2 down
>                                               fib_del_ifaddr
>                                                 fib_table_delete // delete
> fib table
> 
>           ip_neigh_for_gw()
>           |  ip_neigh_gw4()
>           |    __ipv4_neigh_lookup_noref()
>           |    __neigh_create()
>           |      tbl->constructor(n) --> arp_constructor()
>           |        neigh->type = inet_addr_type_dev_table(); // no route,
> neigh->type = RTN_UNICAST
>           neigh_output() // unicast, send an arp request and create an
> exception arp entry
> 
> After the above operation, an abnormal arp entry will be generated. If
> the ip is configured again(ifconfig eth0:2 12.0.208.0), the abnormal arp
> entry will still exist, and the udp broadcast message will be converted
> to an arp request message when it is sent.

Can you try the below? Not really happy with it, but don't have a better
idea at the moment. It seemed better to handle it from the control path
than augmenting the data path with more checks

diff --git a/include/net/arp.h b/include/net/arp.h
index 031374ac2f22..9e6a1961b64c 100644
--- a/include/net/arp.h
+++ b/include/net/arp.h
@@ -64,6 +64,7 @@ void arp_send(int type, int ptype, __be32 dest_ip,
 	      const unsigned char *dest_hw,
 	      const unsigned char *src_hw, const unsigned char *th);
 int arp_mc_map(__be32 addr, u8 *haddr, struct net_device *dev, int dir);
+int arp_invalidate(struct net_device *dev, __be32 ip);
 void arp_ifdown(struct net_device *dev);
 
 struct sk_buff *arp_create(int type, int ptype, __be32 dest_ip,
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 4db0325f6e1a..b81665ce2b57 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -1116,7 +1116,7 @@ static int arp_req_get(struct arpreq *r, struct net_device *dev)
 	return err;
 }
 
-static int arp_invalidate(struct net_device *dev, __be32 ip)
+int arp_invalidate(struct net_device *dev, __be32 ip)
 {
 	struct neighbour *neigh = neigh_lookup(&arp_tbl, &ip, dev);
 	int err = -ENXIO;
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 4d61ddd8a0ec..2d7085232cb5 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -1112,9 +1112,11 @@ void fib_add_ifaddr(struct in_ifaddr *ifa)
 		return;
 
 	/* Add broadcast address, if it is explicitly assigned. */
-	if (ifa->ifa_broadcast && ifa->ifa_broadcast != htonl(0xFFFFFFFF))
+	if (ifa->ifa_broadcast && ifa->ifa_broadcast != htonl(0xFFFFFFFF)) {
 		fib_magic(RTM_NEWROUTE, RTN_BROADCAST, ifa->ifa_broadcast, 32,
 			  prim, 0);
+		arp_invalidate(dev, ifa->ifa_broadcast);
+	}
 
 	if (!ipv4_is_zeronet(prefix) && !(ifa->ifa_flags & IFA_F_SECONDARY) &&
 	    (prefix != addr || ifa->ifa_prefixlen < 32)) {
@@ -1128,6 +1130,7 @@ void fib_add_ifaddr(struct in_ifaddr *ifa)
 		if (ifa->ifa_prefixlen < 31) {
 			fib_magic(RTM_NEWROUTE, RTN_BROADCAST, prefix | ~mask,
 				  32, prim, 0);
+			arp_invalidate(dev, prefix | ~mask);
 		}
 	}
 }

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message
  2022-02-08  7:51 ` Ido Schimmel
@ 2022-02-08  9:18   ` wanghai (M)
  2022-02-16  3:19   ` wanghai (M)
  1 sibling, 0 replies; 6+ messages in thread
From: wanghai (M) @ 2022-02-08  9:18 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: David Miller, Jakub Kicinski, edumazet, yoshfuji, dsahern,
	netdev, linux-kernel


在 2022/2/8 15:51, Ido Schimmel 写道:
> On Mon, Feb 07, 2022 at 10:09:49PM +0800, wanghai (M) wrote:
>> Hello,
>>
>> I found a bug, but I don't know how to fix it. Anyone have some good ideas?
>>
>> This bug will cause udp broadcast messages not to be sent, but instead send
>> non-expected arp request messages.
>>
>> Deleting the ip while sending udp broadcast messages and then configuring
>> the ip again will probably trigger the bug.
>>
>> The following is the timing diagram of the bug, cpu0 sends a broadcast
>> message and cpu1 deletes the routing table at the appropriate time.
>>
>> cpu0                                        cpu1
>> send()
>>    udp_sendmsg()
>>      ip_route_output_flow()
>>      |  fib_lookup()
>>      udp_send_skb()
>>        ip_send_skb()
>>          ip_finish_output2()
>>
>>                                              ifconfig eth0:2 down
>>                                                fib_del_ifaddr
>>                                                  fib_table_delete // delete
>> fib table
>>
>>            ip_neigh_for_gw()
>>            |  ip_neigh_gw4()
>>            |    __ipv4_neigh_lookup_noref()
>>            |    __neigh_create()
>>            |      tbl->constructor(n) --> arp_constructor()
>>            |        neigh->type = inet_addr_type_dev_table(); // no route,
>> neigh->type = RTN_UNICAST
>>            neigh_output() // unicast, send an arp request and create an
>> exception arp entry
>>
>> After the above operation, an abnormal arp entry will be generated. If
>> the ip is configured again(ifconfig eth0:2 12.0.208.0), the abnormal arp
>> entry will still exist, and the udp broadcast message will be converted
>> to an arp request message when it is sent.
> Can you try the below? Not really happy with it, but don't have a better
> idea at the moment. It seemed better to handle it from the control path
> than augmenting the data path with more checks
>
> diff --git a/include/net/arp.h b/include/net/arp.h
> index 031374ac2f22..9e6a1961b64c 100644
> --- a/include/net/arp.h
> +++ b/include/net/arp.h
> @@ -64,6 +64,7 @@ void arp_send(int type, int ptype, __be32 dest_ip,
>   	      const unsigned char *dest_hw,
>   	      const unsigned char *src_hw, const unsigned char *th);
>   int arp_mc_map(__be32 addr, u8 *haddr, struct net_device *dev, int dir);
> +int arp_invalidate(struct net_device *dev, __be32 ip);
>   void arp_ifdown(struct net_device *dev);
>   
>   struct sk_buff *arp_create(int type, int ptype, __be32 dest_ip,
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 4db0325f6e1a..b81665ce2b57 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -1116,7 +1116,7 @@ static int arp_req_get(struct arpreq *r, struct net_device *dev)
>   	return err;
>   }
>   
> -static int arp_invalidate(struct net_device *dev, __be32 ip)
> +int arp_invalidate(struct net_device *dev, __be32 ip)
>   {
>   	struct neighbour *neigh = neigh_lookup(&arp_tbl, &ip, dev);
>   	int err = -ENXIO;
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 4d61ddd8a0ec..2d7085232cb5 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -1112,9 +1112,11 @@ void fib_add_ifaddr(struct in_ifaddr *ifa)
>   		return;
>   
>   	/* Add broadcast address, if it is explicitly assigned. */
> -	if (ifa->ifa_broadcast && ifa->ifa_broadcast != htonl(0xFFFFFFFF))
> +	if (ifa->ifa_broadcast && ifa->ifa_broadcast != htonl(0xFFFFFFFF)) {
>   		fib_magic(RTM_NEWROUTE, RTN_BROADCAST, ifa->ifa_broadcast, 32,
>   			  prim, 0);
> +		arp_invalidate(dev, ifa->ifa_broadcast);
> +	}
>   
>   	if (!ipv4_is_zeronet(prefix) && !(ifa->ifa_flags & IFA_F_SECONDARY) &&
>   	    (prefix != addr || ifa->ifa_prefixlen < 32)) {
> @@ -1128,6 +1130,7 @@ void fib_add_ifaddr(struct in_ifaddr *ifa)
>   		if (ifa->ifa_prefixlen < 31) {
>   			fib_magic(RTM_NEWROUTE, RTN_BROADCAST, prefix | ~mask,
>   				  32, prim, 0);
> +			arp_invalidate(dev, prefix | ~mask);
>   		}
>   	}
>   }

Thank you for your help. I tested it and it solved my problem.

Tested-by: Wang Hai <wanghai38@huawei.com>

> .
>
-- 
Wang Hai


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message
  2022-02-08  7:51 ` Ido Schimmel
  2022-02-08  9:18   ` wanghai (M)
@ 2022-02-16  3:19   ` wanghai (M)
  2022-02-16  7:29     ` Ido Schimmel
  1 sibling, 1 reply; 6+ messages in thread
From: wanghai (M) @ 2022-02-16  3:19 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: David Miller, Jakub Kicinski, edumazet, yoshfuji, dsahern,
	netdev, linux-kernel


在 2022/2/8 15:51, Ido Schimmel 写道:
> On Mon, Feb 07, 2022 at 10:09:49PM +0800, wanghai (M) wrote:
>> Hello,
>>
>> I found a bug, but I don't know how to fix it. Anyone have some good ideas?
>>
>> This bug will cause udp broadcast messages not to be sent, but instead send
>> non-expected arp request messages.
>>
>> Deleting the ip while sending udp broadcast messages and then configuring
>> the ip again will probably trigger the bug.
>>
>> The following is the timing diagram of the bug, cpu0 sends a broadcast
>> message and cpu1 deletes the routing table at the appropriate time.
>>
>> cpu0                                        cpu1
>> send()
>>    udp_sendmsg()
>>      ip_route_output_flow()
>>      |  fib_lookup()
>>      udp_send_skb()
>>        ip_send_skb()
>>          ip_finish_output2()
>>
>>                                              ifconfig eth0:2 down
>>                                                fib_del_ifaddr
>>                                                  fib_table_delete // delete
>> fib table
>>
>>            ip_neigh_for_gw()
>>            |  ip_neigh_gw4()
>>            |    __ipv4_neigh_lookup_noref()
>>            |    __neigh_create()
>>            |      tbl->constructor(n) --> arp_constructor()
>>            |        neigh->type = inet_addr_type_dev_table(); // no route,
>> neigh->type = RTN_UNICAST
>>            neigh_output() // unicast, send an arp request and create an
>> exception arp entry
>>
>> After the above operation, an abnormal arp entry will be generated. If
>> the ip is configured again(ifconfig eth0:2 12.0.208.0), the abnormal arp
>> entry will still exist, and the udp broadcast message will be converted
>> to an arp request message when it is sent.
> Can you try the below? Not really happy with it, but don't have a better
> idea at the moment. It seemed better to handle it from the control path
> than augmenting the data path with more checks
>
> diff --git a/include/net/arp.h b/include/net/arp.h
> index 031374ac2f22..9e6a1961b64c 100644
> --- a/include/net/arp.h
> +++ b/include/net/arp.h
> @@ -64,6 +64,7 @@ void arp_send(int type, int ptype, __be32 dest_ip,
>   	      const unsigned char *dest_hw,
>   	      const unsigned char *src_hw, const unsigned char *th);
>   int arp_mc_map(__be32 addr, u8 *haddr, struct net_device *dev, int dir);
> +int arp_invalidate(struct net_device *dev, __be32 ip);
>   void arp_ifdown(struct net_device *dev);
>   
>   struct sk_buff *arp_create(int type, int ptype, __be32 dest_ip,
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 4db0325f6e1a..b81665ce2b57 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -1116,7 +1116,7 @@ static int arp_req_get(struct arpreq *r, struct net_device *dev)
>   	return err;
>   }
>   
> -static int arp_invalidate(struct net_device *dev, __be32 ip)
> +int arp_invalidate(struct net_device *dev, __be32 ip)
>   {
>   	struct neighbour *neigh = neigh_lookup(&arp_tbl, &ip, dev);
>   	int err = -ENXIO;
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 4d61ddd8a0ec..2d7085232cb5 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -1112,9 +1112,11 @@ void fib_add_ifaddr(struct in_ifaddr *ifa)
>   		return;
>   
>   	/* Add broadcast address, if it is explicitly assigned. */
> -	if (ifa->ifa_broadcast && ifa->ifa_broadcast != htonl(0xFFFFFFFF))
> +	if (ifa->ifa_broadcast && ifa->ifa_broadcast != htonl(0xFFFFFFFF)) {
>   		fib_magic(RTM_NEWROUTE, RTN_BROADCAST, ifa->ifa_broadcast, 32,
>   			  prim, 0);
> +		arp_invalidate(dev, ifa->ifa_broadcast);
> +	}
>   
>   	if (!ipv4_is_zeronet(prefix) && !(ifa->ifa_flags & IFA_F_SECONDARY) &&
>   	    (prefix != addr || ifa->ifa_prefixlen < 32)) {
> @@ -1128,6 +1130,7 @@ void fib_add_ifaddr(struct in_ifaddr *ifa)
>   		if (ifa->ifa_prefixlen < 31) {
>   			fib_magic(RTM_NEWROUTE, RTN_BROADCAST, prefix | ~mask,
>   				  32, prim, 0);
> +			arp_invalidate(dev, prefix | ~mask);
>   		}
>   	}
>   }
> .

Hi, Schimmel.

can you push this patch to the linux.git tree please?

>
-- 
Wang Hai


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message
  2022-02-16  3:19   ` wanghai (M)
@ 2022-02-16  7:29     ` Ido Schimmel
  2022-02-16  8:03       ` wanghai (M)
  0 siblings, 1 reply; 6+ messages in thread
From: Ido Schimmel @ 2022-02-16  7:29 UTC (permalink / raw)
  To: wanghai (M)
  Cc: David Miller, Jakub Kicinski, edumazet, yoshfuji, dsahern,
	netdev, linux-kernel

On Wed, Feb 16, 2022 at 11:19:01AM +0800, wanghai (M) wrote:
> can you push this patch to the linux.git tree please?

Yes. Will post the patch and a test later this week.

BTW, since it's not fixing a regression (scenario never worked AFAICT) I
will target it at net-next.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message
  2022-02-16  7:29     ` Ido Schimmel
@ 2022-02-16  8:03       ` wanghai (M)
  0 siblings, 0 replies; 6+ messages in thread
From: wanghai (M) @ 2022-02-16  8:03 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: David Miller, Jakub Kicinski, edumazet, yoshfuji, dsahern,
	netdev, linux-kernel


在 2022/2/16 15:29, Ido Schimmel 写道:
> On Wed, Feb 16, 2022 at 11:19:01AM +0800, wanghai (M) wrote:
>> can you push this patch to the linux.git tree please?
> Yes. Will post the patch and a test later this week.
>
> BTW, since it's not fixing a regression (scenario never worked AFAICT) I
> will target it at net-next.
Ok, thank you for your help!
> .
>
-- 
Wang Hai


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-02-16  8:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-07 14:09 [BUG] net: ipv4: The sent udp broadcast message may be converted to an arp request message wanghai (M)
2022-02-08  7:51 ` Ido Schimmel
2022-02-08  9:18   ` wanghai (M)
2022-02-16  3:19   ` wanghai (M)
2022-02-16  7:29     ` Ido Schimmel
2022-02-16  8:03       ` wanghai (M)

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.