| Message ID | 20190225041243.9753-1-liuhangbin@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] sit: use ipv6_mod_enabled to check if ipv6 is disabled | expand |
On 02/24/2019 08:12 PM, Hangbin Liu wrote: > ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled > at running time. > Why is it better exactly ? IPv6 can be enabled on the host, but disabled per device /proc/sys/net/ipv6/conf/{name}/disable_ipv6 > Fixes: 173656accaf5 ("sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach()") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > net/ipv6/sit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c > index e8a1dabef803..eea1f19985a7 100644 > --- a/net/ipv6/sit.c > +++ b/net/ipv6/sit.c > @@ -546,7 +546,7 @@ static int ipip6_err(struct sk_buff *skb, u32 info) > } > > err = 0; > - if (__in6_dev_get(skb->dev) && > + if (ipv6_mod_enabled() && > !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) > goto out; > >
On Sun, Feb 24, 2019 at 08:24:51PM -0800, Eric Dumazet wrote: > > > On 02/24/2019 08:12 PM, Hangbin Liu wrote: > > ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled > > at running time. > > > > > Why is it better exactly ? > > IPv6 can be enabled on the host, but disabled per device > > /proc/sys/net/ipv6/conf/{name}/disable_ipv6 Sorry, it looks I didn't make it clear in the commit description. This issue only occurs when IPv6 is disabled at boot time as there is no IPv6 route entry. Disable ipv6 on specific interface is not affected. So check ipv6_mod_enabled() is enough and we don't need to worry about the rcu_read_lock or the dev status. Should I update the commit description? Thanks Hangbin > > > > Fixes: 173656accaf5 ("sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach()") > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > --- > > net/ipv6/sit.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c > > index e8a1dabef803..eea1f19985a7 100644 > > --- a/net/ipv6/sit.c > > +++ b/net/ipv6/sit.c > > @@ -546,7 +546,7 @@ static int ipip6_err(struct sk_buff *skb, u32 info) > > } > > > > err = 0; > > - if (__in6_dev_get(skb->dev) && > > + if (ipv6_mod_enabled() && > > !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) > > goto out; > > > >
On 02/25/2019 12:17 AM, Hangbin Liu wrote: > On Sun, Feb 24, 2019 at 08:24:51PM -0800, Eric Dumazet wrote: >> >> >> On 02/24/2019 08:12 PM, Hangbin Liu wrote: >>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled >>> at running time. >>> >> >> >> Why is it better exactly ? >> >> IPv6 can be enabled on the host, but disabled per device >> >> /proc/sys/net/ipv6/conf/{name}/disable_ipv6 > > Sorry, it looks I didn't make it clear in the commit description. > This issue only occurs when IPv6 is disabled at boot time as there is > no IPv6 route entry. Disable ipv6 on specific interface is not affected. > So check ipv6_mod_enabled() is enough and we don't need to worry about > the rcu_read_lock or the dev status. > > Should I update the commit description? Certainly. Are you telling us skb->dev could be NULL here ? Because rcu_read_lock() should already be asserted.
On 2/25/19 9:39 AM, Eric Dumazet wrote: > > > On 02/25/2019 12:17 AM, Hangbin Liu wrote: >> On Sun, Feb 24, 2019 at 08:24:51PM -0800, Eric Dumazet wrote: >>> >>> >>> On 02/24/2019 08:12 PM, Hangbin Liu wrote: >>>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled >>>> at running time. >>>> >>> >>> >>> Why is it better exactly ? >>> >>> IPv6 can be enabled on the host, but disabled per device >>> >>> /proc/sys/net/ipv6/conf/{name}/disable_ipv6 >> >> Sorry, it looks I didn't make it clear in the commit description. >> This issue only occurs when IPv6 is disabled at boot time as there is >> no IPv6 route entry. Disable ipv6 on specific interface is not affected. >> So check ipv6_mod_enabled() is enough and we don't need to worry about >> the rcu_read_lock or the dev status. >> >> Should I update the commit description? > > Certainly. Are you telling us skb->dev could be NULL here ? > > Because rcu_read_lock() should already be asserted. > Same response as geneve. The existing check is more appropriate and relevant for the code path: is ipv6 enabled on this device versus is ipv6 enabled at all.
On Mon, Feb 25, 2019 at 08:39:27AM -0800, Eric Dumazet wrote: > >> On 02/24/2019 08:12 PM, Hangbin Liu wrote: > >>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled > >>> at running time. > >>> > >> > >> > >> Why is it better exactly ? > >> > >> IPv6 can be enabled on the host, but disabled per device > >> > >> /proc/sys/net/ipv6/conf/{name}/disable_ipv6 > > > > Sorry, it looks I didn't make it clear in the commit description. > > This issue only occurs when IPv6 is disabled at boot time as there is > > no IPv6 route entry. Disable ipv6 on specific interface is not affected. > > So check ipv6_mod_enabled() is enough and we don't need to worry about > > the rcu_read_lock or the dev status. > > > > Should I update the commit description? > > Certainly. Are you telling us skb->dev could be NULL here ? > > Because rcu_read_lock() should already be asserted. > No. I know skb->dev is not NULL and we have rcu_read_lock() here. But can we guarantee the skb->dev won't be NULL forever? Maybe I'm a little sensitive. I mean for only checking if ipv6 is disable at boot time, use ipv6_mod_enabled() is more suitable. Thanks Hangbin
On Mon, Feb 25, 2019 at 09:57:42AM -0700, David Ahern wrote: > On 2/25/19 9:39 AM, Eric Dumazet wrote: > >>> On 02/24/2019 08:12 PM, Hangbin Liu wrote: > >>>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled > >>>> at running time. > >>> > >>> Why is it better exactly ? > >>> > >>> IPv6 can be enabled on the host, but disabled per device > >>> > >>> /proc/sys/net/ipv6/conf/{name}/disable_ipv6 > >> > >> Sorry, it looks I didn't make it clear in the commit description. > >> This issue only occurs when IPv6 is disabled at boot time as there is > >> no IPv6 route entry. Disable ipv6 on specific interface is not affected. > >> So check ipv6_mod_enabled() is enough and we don't need to worry about > >> the rcu_read_lock or the dev status. > >> > >> Should I update the commit description? > > > > Certainly. Are you telling us skb->dev could be NULL here ? > > > > Because rcu_read_lock() should already be asserted. > > > > Same response as geneve. The existing check is more appropriate and > relevant for the code path: is ipv6 enabled on this device versus is > ipv6 enabled at all. Hi David, Just as I said, this issue only occurs when IPv6 is disabled at boot time as there is no IPv6 route entry. Disable ipv6 on specific interface should not be affected(IPv6 route/fib has inited). So I think use ipv6_mod_enabled() would be more suitable in this scenario. Did I miss something? Thanks Hangbin
On 2/25/19 6:55 PM, Hangbin Liu wrote: > Just as I said, this issue only occurs when IPv6 is disabled at boot time > as there is no IPv6 route entry. Disable ipv6 on specific interface should > not be affected(IPv6 route/fib has inited). So I think use ipv6_mod_enabled() > would be more suitable in this scenario. Did I miss something? From a readability perspective the code path depends on whether ipv6 is enabled on the device. I think it is better to leave that as it is.
Hi David, On Mon, Feb 25, 2019 at 07:15:26PM -0700, David Ahern wrote: > On 2/25/19 6:55 PM, Hangbin Liu wrote: > > Just as I said, this issue only occurs when IPv6 is disabled at boot time > > as there is no IPv6 route entry. Disable ipv6 on specific interface should > > not be affected(IPv6 route/fib has inited). So I think use ipv6_mod_enabled() > > would be more suitable in this scenario. Did I miss something? > > From a readability perspective the code path depends on whether ipv6 is > enabled on the device. I think it is better to leave that as it is. When I posted 173656accaf5 ("sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach()"), my purpose is to check if IPv6 disabled at boot time. I didn't know we have ipv6_mod_enabled() at that time, so I just used __in6_dev_get() as a trick way/work around. A few days later I saw your commit e434863718d4 ("net: vrf: Fix crash when IPv6 is disabled at boot time") and I thought this would be a more clear way to tell people that we are checking if IPv6 disabled at boot time. So I posted these two follow up fixes. I don't know why you thought check IPv6 is enbled on the device is better. Because it's more strict? Maybe I missed something here. But if you feel it is better to leave as it it, then let's keep it. Thanks Hangbin
On 02/25/2019 08:08 PM, Hangbin Liu wrote: > Hi David, > On Mon, Feb 25, 2019 at 07:15:26PM -0700, David Ahern wrote: >> On 2/25/19 6:55 PM, Hangbin Liu wrote: >>> Just as I said, this issue only occurs when IPv6 is disabled at boot time >>> as there is no IPv6 route entry. Disable ipv6 on specific interface should >>> not be affected(IPv6 route/fib has inited). So I think use ipv6_mod_enabled() >>> would be more suitable in this scenario. Did I miss something? >> >> From a readability perspective the code path depends on whether ipv6 is >> enabled on the device. I think it is better to leave that as it is. > > When I posted 173656accaf5 ("sit: check if IPv6 enabled before calling > ip6_err_gen_icmpv6_unreach()"), my purpose is to check if IPv6 disabled > at boot time. I didn't know we have ipv6_mod_enabled() at that time, so I > just used __in6_dev_get() as a trick way/work around. > > A few days later I saw your commit e434863718d4 ("net: vrf: Fix crash when > IPv6 is disabled at boot time") and I thought this would be a more clear way > to tell people that we are checking if IPv6 disabled at boot time. So I posted > these two follow up fixes. > > I don't know why you thought check IPv6 is enbled on the device is better. > Because it's more strict? Maybe I missed something here. But if you feel it > is better to leave as it it, then let's keep it. > I do not get it. We really do not care if IPv6 is enabled on some device on the host. Here the correct test is checking if IPv6 is enabled for _this_ device. It is not about fixing a crash (it is already fixed), but not having to call ip6_err_gen_icmpv6_unreach() knowing it will fail anyway. So the current code is better. Thank you.
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c index e8a1dabef803..eea1f19985a7 100644 --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -546,7 +546,7 @@ static int ipip6_err(struct sk_buff *skb, u32 info) } err = 0; - if (__in6_dev_get(skb->dev) && + if (ipv6_mod_enabled() && !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) goto out;
ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled at running time. Fixes: 173656accaf5 ("sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach()") Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- net/ipv6/sit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)