[PATCHv2 net-next 0/2] route: add support and selftests for directed broadcast forwarding

[PATCHv2 net-next 0/2] route: add support and selftests for directed broadcast forwarding

[Xin Long]

Patch 1/2 is the feature and 2/2 is the selftest. Check the changelog
on each of them to know the details.

v1->v2:
  - fix a typo in changelog.
  - fix an uapi break that Davide noticed.
  - flush route cache when bc_forwarding is changed.
  - add the selftest for this patch as Ido's suggestion.

Xin Long (2):
  route: add support for directed broadcast forwarding
  selftests: add a selftest for directed broadcast forwarding

 include/linux/inetdevice.h                         |   1 +
 include/uapi/linux/ip.h                            |   1 +
 include/uapi/linux/netconf.h                       |   1 +
 net/ipv4/devinet.c                                 |  11 ++
 net/ipv4/route.c                                   |   6 +-
 .../selftests/net/forwarding/router_broadcast.sh   | 142 +++++++++++++++++++++
 6 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/forwarding/router_broadcast.sh

-- 
2.1.0

[PATCHv2 net-next 1/2] route: add support for directed broadcast forwarding

[Xin Long]

This patch implements the feature described in rfc1812#section-5.3.5.2
and rfc2644. It allows the router to forward directed broadcast when
sysctl bc_forwarding is enabled.

Note that this feature could be done by iptables -j TEE, but it would
cause some problems:
  - target TEE's gateway param has to be set with a specific address,
    and it's not flexible especially when the route wants forward all
    directed broadcasts.
  - this duplicates the directed broadcasts so this may cause side
    effects to applications.

Besides, to keep consistent with other os router like BSD, it's also
necessary to implement it in the route rx path.

Note that route cache needs to be flushed when bc_forwarding is
changed.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/linux/inetdevice.h   |  1 +
 include/uapi/linux/ip.h      |  1 +
 include/uapi/linux/netconf.h |  1 +
 net/ipv4/devinet.c           | 11 +++++++++++
 net/ipv4/route.c             |  6 +++++-
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 27650f1..c759d1c 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -93,6 +93,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
 
 #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
 #define IN_DEV_MFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), MC_FORWARDING)
+#define IN_DEV_BFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), BC_FORWARDING)
 #define IN_DEV_RPFILTER(in_dev)		IN_DEV_MAXCONF((in_dev), RP_FILTER)
 #define IN_DEV_SRC_VMARK(in_dev)    	IN_DEV_ORCONF((in_dev), SRC_VMARK)
 #define IN_DEV_SOURCE_ROUTE(in_dev)	IN_DEV_ANDCONF((in_dev), \
diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
index b24a742..e42d13b 100644
--- a/include/uapi/linux/ip.h
+++ b/include/uapi/linux/ip.h
@@ -168,6 +168,7 @@ enum
 	IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN,
 	IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST,
 	IPV4_DEVCONF_DROP_GRATUITOUS_ARP,
+	IPV4_DEVCONF_BC_FORWARDING,
 	__IPV4_DEVCONF_MAX
 };
 
diff --git a/include/uapi/linux/netconf.h b/include/uapi/linux/netconf.h
index c84fcdf..fac4edd 100644
--- a/include/uapi/linux/netconf.h
+++ b/include/uapi/linux/netconf.h
@@ -18,6 +18,7 @@ enum {
 	NETCONFA_PROXY_NEIGH,
 	NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
 	NETCONFA_INPUT,
+	NETCONFA_BC_FORWARDING,
 	__NETCONFA_MAX
 };
 #define NETCONFA_MAX	(__NETCONFA_MAX - 1)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d7585ab..80cb464 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1827,6 +1827,8 @@ static int inet_netconf_msgsize_devconf(int type)
 		size += nla_total_size(4);
 	if (all || type == NETCONFA_MC_FORWARDING)
 		size += nla_total_size(4);
+	if (all || type == NETCONFA_BC_FORWARDING)
+		size += nla_total_size(4);
 	if (all || type == NETCONFA_PROXY_NEIGH)
 		size += nla_total_size(4);
 	if (all || type == NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN)
@@ -1873,6 +1875,10 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
 			IPV4_DEVCONF(*devconf, MC_FORWARDING)) < 0)
 		goto nla_put_failure;
+	if ((all || type == NETCONFA_BC_FORWARDING) &&
+	    nla_put_s32(skb, NETCONFA_BC_FORWARDING,
+			IPV4_DEVCONF(*devconf, BC_FORWARDING)) < 0)
+		goto nla_put_failure;
 	if ((all || type == NETCONFA_PROXY_NEIGH) &&
 	    nla_put_s32(skb, NETCONFA_PROXY_NEIGH,
 			IPV4_DEVCONF(*devconf, PROXY_ARP)) < 0)
@@ -2143,6 +2149,10 @@ static int devinet_conf_proc(struct ctl_table *ctl, int write,
 			if ((new_value == 0) && (old_value != 0))
 				rt_cache_flush(net);
 
+		if (i == IPV4_DEVCONF_BC_FORWARDING - 1 ||
+		    new_value != old_value)
+			rt_cache_flush(net);
+
 		if (i == IPV4_DEVCONF_RP_FILTER - 1 &&
 		    new_value != old_value) {
 			ifindex = devinet_conf_ifindex(net, cnf);
@@ -2259,6 +2269,7 @@ static struct devinet_sysctl_table {
 		DEVINET_SYSCTL_COMPLEX_ENTRY(FORWARDING, "forwarding",
 					     devinet_sysctl_forward),
 		DEVINET_SYSCTL_RO_ENTRY(MC_FORWARDING, "mc_forwarding"),
+		DEVINET_SYSCTL_RW_ENTRY(BC_FORWARDING, "bc_forwarding"),
 
 		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_REDIRECTS, "accept_redirects"),
 		DEVINET_SYSCTL_RW_ENTRY(SECURE_REDIRECTS, "secure_redirects"),
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 1df6e97..b678466 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1996,8 +1996,11 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 		goto no_route;
 	}
 
-	if (res->type == RTN_BROADCAST)
+	if (res->type == RTN_BROADCAST) {
+		if (IN_DEV_BFORWARD(in_dev))
+			goto make_route;
 		goto brd_input;
+	}
 
 	if (res->type == RTN_LOCAL) {
 		err = fib_validate_source(skb, saddr, daddr, tos,
@@ -2014,6 +2017,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	if (res->type != RTN_UNICAST)
 		goto martian_destination;
 
+make_route:
 	err = ip_mkroute_input(skb, res, in_dev, daddr, saddr, tos, flkeys);
 out:	return err;
 
-- 
2.1.0

[PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

As Ido's suggestion, this patch is to add a selftest for directed
broadcast forwarding with vrf. Just note that it puts the h2 into
the main route space, so that ping_test could get echo_reply.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 .../selftests/net/forwarding/router_broadcast.sh   | 142 +++++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/router_broadcast.sh

diff --git a/tools/testing/selftests/net/forwarding/router_broadcast.sh b/tools/testing/selftests/net/forwarding/router_broadcast.sh
new file mode 100755
index 0000000..6917768
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/router_broadcast.sh
@@ -0,0 +1,142 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="ping_ipv4"
+NUM_NETIFS=4
+source lib.sh
+
+h1_create()
+{
+	vrf_create "vrf-h1"
+	ip link set dev $h1 master vrf-h1
+
+	ip link set dev vrf-h1 up
+	ip link set dev $h1 up
+
+	ip address add 192.0.2.2/24 dev $h1
+	ip route add 198.51.100.0/24 vrf vrf-h1 nexthop via 192.0.2.1
+}
+
+h1_destroy()
+{
+	ip route del 198.51.100.0/24 vrf vrf-h1
+	ip address del 192.0.2.2/24 dev $h1
+
+	ip link set dev $h1 down
+	vrf_destroy "vrf-h1"
+}
+
+h2_create()
+{
+	ip link set dev $h2 up
+
+	ip address add 198.51.100.2/24 dev $h2
+	ip route add 192.0.2.0/24 dev $h2 via 198.51.100.1
+}
+
+h2_destroy()
+{
+	ip route del 192.0.2.0/24 dev $h2 via 198.51.100.1
+	ip address del 198.51.100.2/24 dev $h2
+
+	ip link set dev $h2 down
+}
+
+router_create()
+{
+	vrf_create "vrf-r1"
+	ip link set dev $rp1 master vrf-r1
+	ip link set dev $rp2 master vrf-r1
+
+	ip link set dev vrf-r1 up
+	ip link set dev $rp1 up
+	ip link set dev $rp2 up
+
+	ip address add 192.0.2.1/24 dev $rp1
+	ip address add 198.51.100.1/24 dev $rp2
+}
+
+router_destroy()
+{
+	ip address del 198.51.100.1/24 dev $rp2
+	ip address del 192.0.2.1/24 dev $rp1
+
+	ip link set dev $rp2 down
+	ip link set dev $rp1 down
+	vrf_destroy "vrf-r1"
+}
+
+bc_forwarding_disable()
+{
+	sysctl_set net.ipv4.conf.all.bc_forwarding 0
+	sysctl_set net.ipv4.conf.$rp1.bc_forwarding 0
+}
+
+bc_forwarding_enable()
+{
+	sysctl_set net.ipv4.conf.all.bc_forwarding 1
+	sysctl_set net.ipv4.conf.$rp1.bc_forwarding 1
+}
+
+bc_forwarding_restore()
+{
+	sysctl_restore net.ipv4.conf.$rp1.bc_forwarding
+	sysctl_restore net.ipv4.conf.all.bc_forwarding
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	rp1=${NETIFS[p2]}
+
+	rp2=${NETIFS[p3]}
+	h2=${NETIFS[p4]}
+
+	vrf_prepare
+
+	h1_create
+	h2_create
+
+	router_create
+
+	forwarding_enable
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	forwarding_restore
+
+	router_destroy
+
+	h2_destroy
+	h1_destroy
+
+	vrf_cleanup
+}
+
+ping_ipv4()
+{
+	sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
+	bc_forwarding_disable
+	ping_test $h1 198.51.100.255
+
+	iptables -A INPUT -i vrf-r1 -p icmp -j DROP
+	bc_forwarding_restore
+	bc_forwarding_enable
+	ping_test $h1 198.51.100.255
+
+	bc_forwarding_restore
+	iptables -D INPUT -i vrf-r1 -p icmp -j DROP
+	sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.1.0

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[David Ahern]

On 7/2/18 12:30 AM, Xin Long wrote:
> +ping_ipv4()
> +{
> +	sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
> +	bc_forwarding_disable
> +	ping_test $h1 198.51.100.255
> +
> +	iptables -A INPUT -i vrf-r1 -p icmp -j DROP
> +	bc_forwarding_restore
> +	bc_forwarding_enable
> +	ping_test $h1 198.51.100.255
> +
> +	bc_forwarding_restore
> +	iptables -D INPUT -i vrf-r1 -p icmp -j DROP
> +	sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
> +}

Both tests fail for me:
TEST: ping                                              [FAIL]
TEST: ping                                              [FAIL]

Why the need for the iptables rule?

And, PAUSE_ON_FAIL is not working to take a look at why tests are
failing. e.g.,

PAUSE_ON_FAIL=yes ./router_broadcast.sh

just continues on. Might be something with the infrastructure scripts.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

On Mon, Jul 2, 2018 at 11:12 PM, David Ahern <dsahern@gmail.com> wrote:
> On 7/2/18 12:30 AM, Xin Long wrote:
>> +ping_ipv4()
>> +{
>> +     sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
>> +     bc_forwarding_disable
>> +     ping_test $h1 198.51.100.255
>> +
>> +     iptables -A INPUT -i vrf-r1 -p icmp -j DROP
>> +     bc_forwarding_restore
>> +     bc_forwarding_enable
>> +     ping_test $h1 198.51.100.255
>> +
>> +     bc_forwarding_restore
>> +     iptables -D INPUT -i vrf-r1 -p icmp -j DROP
>> +     sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
>> +}
>
> Both tests fail for me:
> TEST: ping                                              [FAIL]
> TEST: ping                                              [FAIL]
I think 'ip vrf exec ...' is not working in your env, while
the testing is using "ip vrf exec vrf-h1 ping ..."

You can test it by:
# ip link add dev vrf-test type vrf table 1111
# ip vrf exec vrf-test ls

>
> Why the need for the iptables rule?
This iptables rule is to block the echo_request packet going to
route's local_in.
When bc_forwarding is NOT doing forwarding well but the packet
goes to the route's local_in, it will fail.

Without this rule, the 2nd ping will always succeed, we can't tell the
echo_reply is from route or h2.

Or you have a better way to test this?

>
> And, PAUSE_ON_FAIL is not working to take a look at why tests are
> failing. e.g.,
>
> PAUSE_ON_FAIL=yes ./router_broadcast.sh
>
> just continues on. Might be something with the infrastructure scripts.
Yes, in ./router_broadcast.sh, it loads lib.sh where it loads forwarding.config
where it has "PAUSE_ON_FAIL=no", which would override your
"PAUSE_ON_FAIL=yes".

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[David Ahern]

On 7/3/18 5:36 AM, Xin Long wrote:
> On Mon, Jul 2, 2018 at 11:12 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 7/2/18 12:30 AM, Xin Long wrote:
>>> +ping_ipv4()
>>> +{
>>> +     sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
>>> +     bc_forwarding_disable
>>> +     ping_test $h1 198.51.100.255
>>> +
>>> +     iptables -A INPUT -i vrf-r1 -p icmp -j DROP
>>> +     bc_forwarding_restore
>>> +     bc_forwarding_enable
>>> +     ping_test $h1 198.51.100.255
>>> +
>>> +     bc_forwarding_restore
>>> +     iptables -D INPUT -i vrf-r1 -p icmp -j DROP
>>> +     sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
>>> +}
>>
>> Both tests fail for me:
>> TEST: ping                                              [FAIL]
>> TEST: ping                                              [FAIL]
> I think 'ip vrf exec ...' is not working in your env, while
> the testing is using "ip vrf exec vrf-h1 ping ..."
> 
> You can test it by:
> # ip link add dev vrf-test type vrf table 1111
> # ip vrf exec vrf-test ls

well, that's embarrassing. yes, I updated ip and forgot to apply the bpf
workaround to define the syscall number (not defined in jessie).

> 
>>
>> Why the need for the iptables rule?
> This iptables rule is to block the echo_request packet going to
> route's local_in.
> When bc_forwarding is NOT doing forwarding well but the packet
> goes to the route's local_in, it will fail.
> 
> Without this rule, the 2nd ping will always succeed, we can't tell the
> echo_reply is from route or h2.
> 
> Or you have a better way to test this?

your commands are not a proper test. The test should succeed and fail
based on the routing lookup, not iptables rules.

> 
>>
>> And, PAUSE_ON_FAIL is not working to take a look at why tests are
>> failing. e.g.,
>>
>> PAUSE_ON_FAIL=yes ./router_broadcast.sh
>>
>> just continues on. Might be something with the infrastructure scripts.
> Yes, in ./router_broadcast.sh, it loads lib.sh where it loads forwarding.config
> where it has "PAUSE_ON_FAIL=no", which would override your
> "PAUSE_ON_FAIL=yes".
> 

ack. bit by that as well.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

On Wed, Jul 4, 2018 at 3:23 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/3/18 5:36 AM, Xin Long wrote:
>> On Mon, Jul 2, 2018 at 11:12 PM, David Ahern <dsahern@gmail.com> wrote:
>>> On 7/2/18 12:30 AM, Xin Long wrote:
>>>> +ping_ipv4()
>>>> +{
>>>> +     sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
>>>> +     bc_forwarding_disable
>>>> +     ping_test $h1 198.51.100.255
>>>> +
>>>> +     iptables -A INPUT -i vrf-r1 -p icmp -j DROP
>>>> +     bc_forwarding_restore
>>>> +     bc_forwarding_enable
>>>> +     ping_test $h1 198.51.100.255
>>>> +
>>>> +     bc_forwarding_restore
>>>> +     iptables -D INPUT -i vrf-r1 -p icmp -j DROP
>>>> +     sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
>>>> +}
>>>
>>> Both tests fail for me:
>>> TEST: ping                                              [FAIL]
>>> TEST: ping                                              [FAIL]
>> I think 'ip vrf exec ...' is not working in your env, while
>> the testing is using "ip vrf exec vrf-h1 ping ..."
>>
>> You can test it by:
>> # ip link add dev vrf-test type vrf table 1111
>> # ip vrf exec vrf-test ls
>
> well, that's embarrassing. yes, I updated ip and forgot to apply the bpf
> workaround to define the syscall number (not defined in jessie).
>
>>
>>>
>>> Why the need for the iptables rule?
>> This iptables rule is to block the echo_request packet going to
>> route's local_in.
>> When bc_forwarding is NOT doing forwarding well but the packet
>> goes to the route's local_in, it will fail.
>>
>> Without this rule, the 2nd ping will always succeed, we can't tell the
>> echo_reply is from route or h2.
>>
>> Or you have a better way to test this?
>
> your commands are not a proper test. The test should succeed and fail
> based on the routing lookup, not iptables rules.
A proper test can be done easily with netns, as vrf can't isolate much.
I don't want to bother forwarding/ directory with netns, so I will probably
just drop this selftest, and let the feature patch go first.

What do you think?

>
>>
>>>
>>> And, PAUSE_ON_FAIL is not working to take a look at why tests are
>>> failing. e.g.,
>>>
>>> PAUSE_ON_FAIL=yes ./router_broadcast.sh
>>>
>>> just continues on. Might be something with the infrastructure scripts.
>> Yes, in ./router_broadcast.sh, it loads lib.sh where it loads forwarding.config
>> where it has "PAUSE_ON_FAIL=no", which would override your
>> "PAUSE_ON_FAIL=yes".
>>
>
> ack. bit by that as well.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[David Ahern]

On 7/4/18 11:56 AM, Xin Long wrote:
> A proper test can be done easily with netns, as vrf can't isolate much.
> I don't want to bother forwarding/ directory with netns, so I will probably
> just drop this selftest, and let the feature patch go first.
> 
> What do you think?
> 

I think I would like to see a proper test case for this.

If it does not fit the model that Ido and others are using under the
forwarding directory, then how about one in tools/testing/selftests/net
then.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[David Ahern]

On 7/4/18 11:56 AM, Xin Long wrote:

>> your commands are not a proper test. The test should succeed and fail
>> based on the routing lookup, not iptables rules.
> A proper test can be done easily with netns, as vrf can't isolate much.
> I don't want to bother forwarding/ directory with netns, so I will probably
> just drop this selftest, and let the feature patch go first.
> 

BTW, VRF isolates at the routing layer and this is a routing change. We
need to understand why it does not work with VRF. Perhaps another tweak
is needed for VRF.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

On Thu, Jul 5, 2018 at 2:36 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/4/18 11:56 AM, Xin Long wrote:
>
>>> your commands are not a proper test. The test should succeed and fail
>>> based on the routing lookup, not iptables rules.
>> A proper test can be done easily with netns, as vrf can't isolate much.
>> I don't want to bother forwarding/ directory with netns, so I will probably
>> just drop this selftest, and let the feature patch go first.
>>
>
> BTW, VRF isolates at the routing layer and this is a routing change. We
> need to understand why it does not work with VRF. Perhaps another tweak
> is needed for VRF.
One problem was that the peer may not use the address on the dev
that echo_request comes from as the src IP of echo_reply when the
echo_request's dst IP is broadcast, but try to get another one by
looking up a route without ".flowi4_oif" set. See:

icmp_reply()->fib_compute_spec_dst():
                struct flowi4 fl4 = {
                        .flowi4_iif = LOOPBACK_IFINDEX,
                        .daddr = ip_hdr(skb)->saddr,
                        .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
                        .flowi4_scope = scope,
                        .flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
                };
                if (!fib_lookup(net, &fl4, &res, 0))
                        return FIB_RES_PREFSRC(net, res);


Without ".flowi4_oif" set, it won't match the vrf route. That's why
I had to make h2 NOT into a vrf so that h1 can get the echo_reply.
But it can't tell if this echo_reply is from h2 or r1, as r1's echo_reply
will also use the same src IP which is actually got from main route
space as  ".flowi4_oif" is not set.
(hope I this description is clear to you) :)

So i'm not sure if we can do any tweak for VRF.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[David Ahern]

On 7/5/18 1:57 AM, Xin Long wrote:
> On Thu, Jul 5, 2018 at 2:36 AM, David Ahern <dsahern@gmail.com> wrote:
>> On 7/4/18 11:56 AM, Xin Long wrote:
>>
>>>> your commands are not a proper test. The test should succeed and fail
>>>> based on the routing lookup, not iptables rules.
>>> A proper test can be done easily with netns, as vrf can't isolate much.
>>> I don't want to bother forwarding/ directory with netns, so I will probably
>>> just drop this selftest, and let the feature patch go first.
>>>
>>
>> BTW, VRF isolates at the routing layer and this is a routing change. We
>> need to understand why it does not work with VRF. Perhaps another tweak
>> is needed for VRF.
> One problem was that the peer may not use the address on the dev
> that echo_request comes from as the src IP of echo_reply when the
> echo_request's dst IP is broadcast, but try to get another one by
> looking up a route without ".flowi4_oif" set. See:
> 
> icmp_reply()->fib_compute_spec_dst():
>                 struct flowi4 fl4 = {
>                         .flowi4_iif = LOOPBACK_IFINDEX,
>                         .daddr = ip_hdr(skb)->saddr,
>                         .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
>                         .flowi4_scope = scope,
>                         .flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
>                 };
>                 if (!fib_lookup(net, &fl4, &res, 0))
>                         return FIB_RES_PREFSRC(net, res);
> 
> 
> Without ".flowi4_oif" set, it won't match the vrf route. That's why
> I had to make h2 NOT into a vrf so that h1 can get the echo_reply.
> But it can't tell if this echo_reply is from h2 or r1, as r1's echo_reply
> will also use the same src IP which is actually got from main route
> space as  ".flowi4_oif" is not set.
> (hope I this description is clear to you) :)
> 
> So i'm not sure if we can do any tweak for VRF.
> 

Try this:

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index b21833651394..e46cdd310e5f 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -300,6 +300,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
        if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
                struct flowi4 fl4 = {
                        .flowi4_iif = LOOPBACK_IFINDEX,
+                       .flowi4_oif = l3mdev_master_ifindex_rcu(dev),
                        .daddr = ip_hdr(skb)->saddr,
                        .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
                        .flowi4_scope = scope,

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

On Thu, Jul 5, 2018 at 9:18 PM, David Ahern <dsahern@gmail.com> wrote:
> On 7/5/18 1:57 AM, Xin Long wrote:
>> On Thu, Jul 5, 2018 at 2:36 AM, David Ahern <dsahern@gmail.com> wrote:
>>> On 7/4/18 11:56 AM, Xin Long wrote:
>>>
>>>>> your commands are not a proper test. The test should succeed and fail
>>>>> based on the routing lookup, not iptables rules.
>>>> A proper test can be done easily with netns, as vrf can't isolate much.
>>>> I don't want to bother forwarding/ directory with netns, so I will probably
>>>> just drop this selftest, and let the feature patch go first.
>>>>
>>>
>>> BTW, VRF isolates at the routing layer and this is a routing change. We
>>> need to understand why it does not work with VRF. Perhaps another tweak
>>> is needed for VRF.
>> One problem was that the peer may not use the address on the dev
>> that echo_request comes from as the src IP of echo_reply when the
>> echo_request's dst IP is broadcast, but try to get another one by
>> looking up a route without ".flowi4_oif" set. See:
>>
>> icmp_reply()->fib_compute_spec_dst():
>>                 struct flowi4 fl4 = {
>>                         .flowi4_iif = LOOPBACK_IFINDEX,
>>                         .daddr = ip_hdr(skb)->saddr,
>>                         .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
>>                         .flowi4_scope = scope,
>>                         .flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
>>                 };
>>                 if (!fib_lookup(net, &fl4, &res, 0))
>>                         return FIB_RES_PREFSRC(net, res);
>>
>>
>> Without ".flowi4_oif" set, it won't match the vrf route. That's why
>> I had to make h2 NOT into a vrf so that h1 can get the echo_reply.
>> But it can't tell if this echo_reply is from h2 or r1, as r1's echo_reply
>> will also use the same src IP which is actually got from main route
>> space as  ".flowi4_oif" is not set.
>> (hope I this description is clear to you) :)
>>
>> So i'm not sure if we can do any tweak for VRF.
>>
>
> Try this:
>
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index b21833651394..e46cdd310e5f 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -300,6 +300,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>         if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
>                 struct flowi4 fl4 = {
>                         .flowi4_iif = LOOPBACK_IFINDEX,
> +                       .flowi4_oif = l3mdev_master_ifindex_rcu(dev),
>                         .daddr = ip_hdr(skb)->saddr,
>                         .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
>                         .flowi4_scope = scope,
Great, with your fix, I can extend more for this selftest.
but I hope no side effects would be caused.

Thank you.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

On Thu, Jul 5, 2018 at 10:07 PM, Xin Long <lucien.xin@gmail.com> wrote:
> On Thu, Jul 5, 2018 at 9:18 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 7/5/18 1:57 AM, Xin Long wrote:
>>> On Thu, Jul 5, 2018 at 2:36 AM, David Ahern <dsahern@gmail.com> wrote:
>>>> On 7/4/18 11:56 AM, Xin Long wrote:
>>>>
>>>>>> your commands are not a proper test. The test should succeed and fail
>>>>>> based on the routing lookup, not iptables rules.
>>>>> A proper test can be done easily with netns, as vrf can't isolate much.
>>>>> I don't want to bother forwarding/ directory with netns, so I will probably
>>>>> just drop this selftest, and let the feature patch go first.
>>>>>
>>>>
>>>> BTW, VRF isolates at the routing layer and this is a routing change. We
>>>> need to understand why it does not work with VRF. Perhaps another tweak
>>>> is needed for VRF.
>>> One problem was that the peer may not use the address on the dev
>>> that echo_request comes from as the src IP of echo_reply when the
>>> echo_request's dst IP is broadcast, but try to get another one by
>>> looking up a route without ".flowi4_oif" set. See:
>>>
>>> icmp_reply()->fib_compute_spec_dst():
>>>                 struct flowi4 fl4 = {
>>>                         .flowi4_iif = LOOPBACK_IFINDEX,
>>>                         .daddr = ip_hdr(skb)->saddr,
>>>                         .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
>>>                         .flowi4_scope = scope,
>>>                         .flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
>>>                 };
>>>                 if (!fib_lookup(net, &fl4, &res, 0))
>>>                         return FIB_RES_PREFSRC(net, res);
>>>
>>>
>>> Without ".flowi4_oif" set, it won't match the vrf route. That's why
>>> I had to make h2 NOT into a vrf so that h1 can get the echo_reply.
>>> But it can't tell if this echo_reply is from h2 or r1, as r1's echo_reply
>>> will also use the same src IP which is actually got from main route
>>> space as  ".flowi4_oif" is not set.
>>> (hope I this description is clear to you) :)
>>>
>>> So i'm not sure if we can do any tweak for VRF.
>>>
>>
>> Try this:
>>
>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
>> index b21833651394..e46cdd310e5f 100644
>> --- a/net/ipv4/fib_frontend.c
>> +++ b/net/ipv4/fib_frontend.c
>> @@ -300,6 +300,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>>         if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
>>                 struct flowi4 fl4 = {
>>                         .flowi4_iif = LOOPBACK_IFINDEX,
>> +                       .flowi4_oif = l3mdev_master_ifindex_rcu(dev),
>>                         .daddr = ip_hdr(skb)->saddr,
>>                         .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
>>                         .flowi4_scope = scope,
If this patch can be applied, I would be able to make a proper selftest like:

...
ping_test_from()
{
local oif=$1
local dip=$2
local from=$3
local fail=$4

RET=0

ip vrf exec $(master_name_get $oif) \
$PING -I $oif $dip -c 10 -i 0.1 -w 2 -b 2>&1 | grep $from &> /dev/null
check_err_fail $fail $?
log_test "ping_test_from"
}

ping_ipv4()
{
sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0

bc_forwarding_disable
ping_test_from $h1 198.51.100.255 192.0.2.1
ping_test_from $h1 198.51.200.255 192.0.2.1
ping_test_from $h1 192.0.2.255 192.0.2.1
ping_test_from $h1 255.255.255.255 192.0.2.1

ping_test_from $h2 192.0.2.255 198.51.100.1
ping_test_from $h2 198.51.200.255 198.51.100.1
ping_test_from $h2 198.51.100.255 198.51.100.1
ping_test_from $h2 255.255.255.255 198.51.100.1
bc_forwarding_restore

bc_forwarding_enable
ping_test_from $h1 198.51.100.255 198.51.100.2
ping_test_from $h1 198.51.200.255 198.51.200.2
ping_test_from $h1 192.0.2.255 192.0.2.1 1
ping_test_from $h1 255.255.255.255 192.0.2.1

ping_test_from $h2 192.0.2.255 192.0.2.2
ping_test_from $h2 198.51.200.255 198.51.200.2
ping_test_from $h2 198.51.100.255 198.51.100.1 1
ping_test_from $h2 255.255.255.255 198.51.100.1
bc_forwarding_restore

sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
}

> Great, with your fix, I can extend more for this selftest.
> but I hope no side effects would be caused.
>
> Thank you.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[David Ahern]

On 7/6/18 3:50 AM, Xin Long wrote:
>>> Try this:
>>>
>>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
>>> index b21833651394..e46cdd310e5f 100644
>>> --- a/net/ipv4/fib_frontend.c
>>> +++ b/net/ipv4/fib_frontend.c
>>> @@ -300,6 +300,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>>>         if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
>>>                 struct flowi4 fl4 = {
>>>                         .flowi4_iif = LOOPBACK_IFINDEX,
>>> +                       .flowi4_oif = l3mdev_master_ifindex_rcu(dev),
>>>                         .daddr = ip_hdr(skb)->saddr,
>>>                         .flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
>>>                         .flowi4_scope = scope,
> If this patch can be applied, I would be able to make a proper selftest like:

Forgot to send the patch yesterday. Will do so sometime this weekend.

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Ido Schimmel]

On Thu, Jul 05, 2018 at 01:56:23AM +0800, Xin Long wrote:
> On Wed, Jul 4, 2018 at 3:23 AM, David Ahern <dsahern@gmail.com> wrote:
> > your commands are not a proper test. The test should succeed and fail
> > based on the routing lookup, not iptables rules.
> A proper test can be done easily with netns, as vrf can't isolate much.
> I don't want to bother forwarding/ directory with netns, so I will probably
> just drop this selftest, and let the feature patch go first.
> 
> What do you think?

You can add a tc rule on the ingress of h2 and make sure that in the
first case ping succeeds and the tc rule wasn't hit. In the second case
ping should also succeed, but the tc rule should be hit. This is similar
to your original netns test.

You can look at tc_flower.sh for reference and in particular at
tc_check_packets().

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

On Thu, Jul 5, 2018 at 4:39 AM, Ido Schimmel <idosch@idosch.org> wrote:
> On Thu, Jul 05, 2018 at 01:56:23AM +0800, Xin Long wrote:
>> On Wed, Jul 4, 2018 at 3:23 AM, David Ahern <dsahern@gmail.com> wrote:
>> > your commands are not a proper test. The test should succeed and fail
>> > based on the routing lookup, not iptables rules.
>> A proper test can be done easily with netns, as vrf can't isolate much.
>> I don't want to bother forwarding/ directory with netns, so I will probably
>> just drop this selftest, and let the feature patch go first.
>>
>> What do you think?
>
> You can add a tc rule on the ingress of h2 and make sure that in the
> first case ping succeeds and the tc rule wasn't hit. In the second case
> ping should also succeed, but the tc rule should be hit. This is similar
> to your original netns test.
With netns, it will be much easier to use
sysctl net.ipv4.icmp_echo_ignore_broadcasts
to block the echo_request on r1 or h2, and check if ping works.
(this's more like the idea of using 'iptables' above) :D

>
> You can look at tc_flower.sh for reference and in particular at
> tc_check_packets().
This is a way similar idea of using tcpdump, I just feel it's too much,
this test should be an as simple test as route.sh. :)

Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding

[Xin Long]

On Thu, Jul 5, 2018 at 4:21 PM, Xin Long <lucien.xin@gmail.com> wrote:
> On Thu, Jul 5, 2018 at 4:39 AM, Ido Schimmel <idosch@idosch.org> wrote:
>> On Thu, Jul 05, 2018 at 01:56:23AM +0800, Xin Long wrote:
>>> On Wed, Jul 4, 2018 at 3:23 AM, David Ahern <dsahern@gmail.com> wrote:
>>> > your commands are not a proper test. The test should succeed and fail
>>> > based on the routing lookup, not iptables rules.
>>> A proper test can be done easily with netns, as vrf can't isolate much.
>>> I don't want to bother forwarding/ directory with netns, so I will probably
>>> just drop this selftest, and let the feature patch go first.
>>>
>>> What do you think?
>>
>> You can add a tc rule on the ingress of h2 and make sure that in the
>> first case ping succeeds and the tc rule wasn't hit. In the second case
>> ping should also succeed, but the tc rule should be hit. This is similar
>> to your original netns test.
> With netns, it will be much easier to use
> sysctl net.ipv4.icmp_echo_ignore_broadcasts
> to block the echo_request on r1 or h2, and check if ping works.
> (this's more like the idea of using 'iptables' above) :D
>
>>
>> You can look at tc_flower.sh for reference and in particular at
>> tc_check_packets().
Just noticed this doesn't require reply with MZ. that's better.
Thanks.

> This is a way similar idea of using tcpdump, I just feel it's too much,
> this test should be an as simple test as route.sh. :)

Re: [PATCHv2 net-next 1/2] route: add support for directed broadcast forwarding

[Davide Caratti]

On Mon, 2018-07-02 at 14:30 +0800, Xin Long wrote:
> This patch implements the feature described in rfc1812#section-5.3.5.2
> and rfc2644. It allows the router to forward directed broadcast when
> sysctl bc_forwarding is enabled.
> 
> Note that this feature could be done by iptables -j TEE, but it would
> cause some problems:
>   - target TEE's gateway param has to be set with a specific address,
>     and it's not flexible especially when the route wants forward all
>     directed broadcasts.
>   - this duplicates the directed broadcasts so this may cause side
>     effects to applications.
> 
> Besides, to keep consistent with other os router like BSD, it's also
> necessary to implement it in the route rx path.
> 
> Note that route cache needs to be flushed when bc_forwarding is
> changed.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/linux/inetdevice.h   |  1 +
>  include/uapi/linux/ip.h      |  1 +
>  include/uapi/linux/netconf.h |  1 +
>  net/ipv4/devinet.c           | 11 +++++++++++
>  net/ipv4/route.c             |  6 +++++-
>  5 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
> index 27650f1..c759d1c 100644
> --- a/include/linux/inetdevice.h
> +++ b/include/linux/inetdevice.h
> @@ -93,6 +93,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
>  
>  #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
>  #define IN_DEV_MFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), MC_FORWARDING)
> +#define IN_DEV_BFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), BC_FORWARDING)
>  #define IN_DEV_RPFILTER(in_dev)		IN_DEV_MAXCONF((in_dev), RP_FILTER)
>  #define IN_DEV_SRC_VMARK(in_dev)    	IN_DEV_ORCONF((in_dev), SRC_VMARK)
>  #define IN_DEV_SOURCE_ROUTE(in_dev)	IN_DEV_ANDCONF((in_dev), \
> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
> index b24a742..e42d13b 100644
> --- a/include/uapi/linux/ip.h
> +++ b/include/uapi/linux/ip.h
> @@ -168,6 +168,7 @@ enum
>  	IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN,
>  	IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST,
>  	IPV4_DEVCONF_DROP_GRATUITOUS_ARP,
> +	IPV4_DEVCONF_BC_FORWARDING,
>  	__IPV4_DEVCONF_MAX
>  };
>  
> diff --git a/include/uapi/linux/netconf.h b/include/uapi/linux/netconf.h
> index c84fcdf..fac4edd 100644
> --- a/include/uapi/linux/netconf.h
> +++ b/include/uapi/linux/netconf.h
> @@ -18,6 +18,7 @@ enum {
>  	NETCONFA_PROXY_NEIGH,
>  	NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
>  	NETCONFA_INPUT,
> +	NETCONFA_BC_FORWARDING,
>  	__NETCONFA_MAX
>  };
>  #define NETCONFA_MAX	(__NETCONFA_MAX - 1)
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index d7585ab..80cb464 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1827,6 +1827,8 @@ static int inet_netconf_msgsize_devconf(int type)
>  		size += nla_total_size(4);
>  	if (all || type == NETCONFA_MC_FORWARDING)
>  		size += nla_total_size(4);
> +	if (all || type == NETCONFA_BC_FORWARDING)
> +		size += nla_total_size(4);
>  	if (all || type == NETCONFA_PROXY_NEIGH)
>  		size += nla_total_size(4);
>  	if (all || type == NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN)
> @@ -1873,6 +1875,10 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>  	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
>  			IPV4_DEVCONF(*devconf, MC_FORWARDING)) < 0)
>  		goto nla_put_failure;
> +	if ((all || type == NETCONFA_BC_FORWARDING) &&
> +	    nla_put_s32(skb, NETCONFA_BC_FORWARDING,
> +			IPV4_DEVCONF(*devconf, BC_FORWARDING)) < 0)
> +		goto nla_put_failure;
>  	if ((all || type == NETCONFA_PROXY_NEIGH) &&
>  	    nla_put_s32(skb, NETCONFA_PROXY_NEIGH,
>  			IPV4_DEVCONF(*devconf, PROXY_ARP)) < 0)
> @@ -2143,6 +2149,10 @@ static int devinet_conf_proc(struct ctl_table *ctl, int write,
>  			if ((new_value == 0) && (old_value != 0))
>  				rt_cache_flush(net);
>  
> +		if (i == IPV4_DEVCONF_BC_FORWARDING - 1 ||
> +		    new_value != old_value)
> +			rt_cache_flush(net);
> +
>  		if (i == IPV4_DEVCONF_RP_FILTER - 1 &&
>  		    new_value != old_value) {
>  			ifindex = devinet_conf_ifindex(net, cnf);
> @@ -2259,6 +2269,7 @@ static struct devinet_sysctl_table {
>  		DEVINET_SYSCTL_COMPLEX_ENTRY(FORWARDING, "forwarding",
>  					     devinet_sysctl_forward),
>  		DEVINET_SYSCTL_RO_ENTRY(MC_FORWARDING, "mc_forwarding"),
> +		DEVINET_SYSCTL_RW_ENTRY(BC_FORWARDING, "bc_forwarding"),
>  
>  		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_REDIRECTS, "accept_redirects"),
>  		DEVINET_SYSCTL_RW_ENTRY(SECURE_REDIRECTS, "secure_redirects"),
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 1df6e97..b678466 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1996,8 +1996,11 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
>  		goto no_route;
>  	}
>  
> -	if (res->type == RTN_BROADCAST)
> +	if (res->type == RTN_BROADCAST) {
> +		if (IN_DEV_BFORWARD(in_dev))
> +			goto make_route;
>  		goto brd_input;
> +	}
>  
>  	if (res->type == RTN_LOCAL) {
>  		err = fib_validate_source(skb, saddr, daddr, tos,
> @@ -2014,6 +2017,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
>  	if (res->type != RTN_UNICAST)
>  		goto martian_destination;
>  
> +make_route:
>  	err = ip_mkroute_input(skb, res, in_dev, daddr, saddr, tos, flkeys);
>  out:	return err;
>  

Acked-by: Davide Caratti <dcaratti@redhat.com>

Re: [PATCHv2 net-next 1/2] route: add support for directed broadcast forwarding

[David Ahern]

On 7/2/18 12:30 AM, Xin Long wrote:
> @@ -2143,6 +2149,10 @@ static int devinet_conf_proc(struct ctl_table *ctl, int write,
>  			if ((new_value == 0) && (old_value != 0))
>  				rt_cache_flush(net);
>  
> +		if (i == IPV4_DEVCONF_BC_FORWARDING - 1 ||
> +		    new_value != old_value)
> +			rt_cache_flush(net);
> +

Shouldn't that be '&&' instead of '||' otherwise you are bumping the fib
gen id any time the old and new values do not equal regardless of which
setting is changed.

Re: [PATCHv2 net-next 1/2] route: add support for directed broadcast forwarding

[Xin Long]

On Mon, Jul 2, 2018 at 11:05 PM, David Ahern <dsahern@gmail.com> wrote:
> On 7/2/18 12:30 AM, Xin Long wrote:
>> @@ -2143,6 +2149,10 @@ static int devinet_conf_proc(struct ctl_table *ctl, int write,
>>                       if ((new_value == 0) && (old_value != 0))
>>                               rt_cache_flush(net);
>>
>> +             if (i == IPV4_DEVCONF_BC_FORWARDING - 1 ||
>> +                 new_value != old_value)
>> +                     rt_cache_flush(net);
>> +
>
> Shouldn't that be '&&' instead of '||' otherwise you are bumping the fib
> gen id any time the old and new values do not equal regardless of which
> setting is changed.
ah right, It's a copy-paste mistake, sorry