[PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Hangbin Liu]

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(-)

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;
 
-- 
2.19.2

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Eric Dumazet]

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;
>  
> 

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Hangbin Liu]

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;
> >  
> > 

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Eric Dumazet]

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.

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[David Ahern]

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.

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Hangbin Liu]

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

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[David Ahern]

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.

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Hangbin Liu]

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

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Eric Dumazet]

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.

Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled

[Hangbin Liu]

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