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);
}
}
}
在 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
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.
?? 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