[PATCH 1/2] gre: fix hard header destination address checking

[PATCH 1/2] gre: fix hard header destination address checking

[Timo Teras]

ipgre_header() can be called with zero daddr when the gre device is
configured as multipoint tunnel and still has the NOARP flag set (which is
typically cleared by the userspace arp daemon).  If the NOARP packets are
not dropped, ipgre_tunnel_xmit() will take rt->rt_gateway (= NBMA IP) and
use that for route look up (and may lead to bogus xfrm acquires).

The multicast address check is removed as sending to multicast group should
be ok.  In fact, if gre device has a multicast address as destination
ipgre_header is always called with multicast address.

Signed-off-by: Timo Teras <timo.teras@iki.fi>
---
 net/ipv4/ip_gre.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index c0c5274..f47c9f7 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1144,12 +1144,9 @@ static int ipgre_header(struct sk_buff *skb, struct net_device *dev,
 
 	if (saddr)
 		memcpy(&iph->saddr, saddr, 4);
-
-	if (daddr) {
+	if (daddr)
 		memcpy(&iph->daddr, daddr, 4);
-		return t->hlen;
-	}
-	if (iph->daddr && !ipv4_is_multicast(iph->daddr))
+	if (iph->daddr)
 		return t->hlen;
 
 	return -t->hlen;
-- 
1.6.3.3

[PATCH 2/2] ipv4: flush ARP entries on device change

[Timo Teras]

If device flag IFF_NOARP is changed, we should flush the ARP cache as all
entries need to get refreshed.

Signed-off-by: Timo Teras <timo.teras@iki.fi>
---
 net/ipv4/arp.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index c4dd135..036da92 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -1245,6 +1245,9 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event, vo
 		neigh_changeaddr(&arp_tbl, dev);
 		rt_cache_flush(dev_net(dev), 0);
 		break;
+	case NETDEV_CHANGE:
+		neigh_changeaddr(&arp_tbl, dev);
+		break;
 	default:
 		break;
 	}
-- 
1.6.3.3

Re: [PATCH 2/2] ipv4: flush ARP entries on device change

[Patrick McHardy]

Timo Teras wrote:
> If device flag IFF_NOARP is changed, we should flush the ARP cache as all
> entries need to get refreshed.
> 
> Signed-off-by: Timo Teras <timo.teras@iki.fi>
> ---
>  net/ipv4/arp.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index c4dd135..036da92 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -1245,6 +1245,9 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event, vo
>  		neigh_changeaddr(&arp_tbl, dev);
>  		rt_cache_flush(dev_net(dev), 0);
>  		break;
> +	case NETDEV_CHANGE:
> +		neigh_changeaddr(&arp_tbl, dev);
> +		break;

It would be nice if we could restrict this to IFF_NOARP changes.

Re: [PATCH 2/2] ipv4: flush ARP entries on device change

[Timo Teräs]

Patrick McHardy wrote:
> Timo Teras wrote:
>> If device flag IFF_NOARP is changed, we should flush the ARP cache as all
>> entries need to get refreshed.
>>
>> Signed-off-by: Timo Teras <timo.teras@iki.fi>
>> ---
>>  net/ipv4/arp.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
>> index c4dd135..036da92 100644
>> --- a/net/ipv4/arp.c
>> +++ b/net/ipv4/arp.c
>> @@ -1245,6 +1245,9 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event, vo
>>  		neigh_changeaddr(&arp_tbl, dev);
>>  		rt_cache_flush(dev_net(dev), 0);
>>  		break;
>> +	case NETDEV_CHANGE:
>> +		neigh_changeaddr(&arp_tbl, dev);
>> +		break;
> 
> It would be nice if we could restrict this to IFF_NOARP changes.

Yes. But I did not see any easy way to figure out which flags have changed.

Should we just keep a copy of the previous IFF_NOARP bit somewhere (where?).
Or did I miss something obvious?

- Timo

Re: [PATCH 2/2] ipv4: flush ARP entries on device change

[Patrick McHardy]

Timo Teräs wrote:
> Patrick McHardy wrote:
>> Timo Teras wrote:
>>> If device flag IFF_NOARP is changed, we should flush the ARP cache as
>>> all
>>> entries need to get refreshed.
>>>
>>> Signed-off-by: Timo Teras <timo.teras@iki.fi>
>>> ---
>>>  net/ipv4/arp.c |    3 +++
>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
>>> index c4dd135..036da92 100644
>>> --- a/net/ipv4/arp.c
>>> +++ b/net/ipv4/arp.c
>>> @@ -1245,6 +1245,9 @@ static int arp_netdev_event(struct
>>> notifier_block *this, unsigned long event, vo
>>>          neigh_changeaddr(&arp_tbl, dev);
>>>          rt_cache_flush(dev_net(dev), 0);
>>>          break;
>>> +    case NETDEV_CHANGE:
>>> +        neigh_changeaddr(&arp_tbl, dev);
>>> +        break;
>>
>> It would be nice if we could restrict this to IFF_NOARP changes.
> 
> Yes. But I did not see any easy way to figure out which flags have changed.
> 
> Should we just keep a copy of the previous IFF_NOARP bit somewhere
> (where?).
> Or did I miss something obvious?

We shouldn't have any arp entries for devices with IFF_NOARP set,
so perhaps we can flush only in that case. The transition IFF_NOARP
-> ~IFF_NOARP shouldn't need flushing.

Re: [PATCH 2/2] ipv4: flush ARP entries on device change

[Timo Teräs]

Patrick McHardy wrote:
> Timo Teräs wrote:
>> Patrick McHardy wrote:
>>> Timo Teras wrote:
>>> It would be nice if we could restrict this to IFF_NOARP changes.
>> Yes. But I did not see any easy way to figure out which flags have changed.
>>
>> Should we just keep a copy of the previous IFF_NOARP bit somewhere
>> (where?).
>> Or did I miss something obvious?
> 
> We shouldn't have any arp entries for devices with IFF_NOARP set,
> so perhaps we can flush only in that case. The transition IFF_NOARP
> -> ~IFF_NOARP shouldn't need flushing.

IFF_NOARP devices do have neighbor entries with the nud NOARP.
Exactly those entries I want to flush when IFF_NOARP flag is
removed.

You can see those entries with "ip neigh show nud all". You have
them e.g. for loopback stuff and broad-/multicast stuff in general.
With IFF_NOARP you get them on all unicast addresses used.

- Timo

Re: [PATCH 2/2] ipv4: flush ARP entries on device change

[Patrick McHardy]

Timo Teräs wrote:
> Patrick McHardy wrote:
>> Timo Teräs wrote:
>>> Patrick McHardy wrote:
>>>> Timo Teras wrote:
>>>> It would be nice if we could restrict this to IFF_NOARP changes.
>>> Yes. But I did not see any easy way to figure out which flags have
>>> changed.
>>>
>>> Should we just keep a copy of the previous IFF_NOARP bit somewhere
>>> (where?).
>>> Or did I miss something obvious?
>>
>> We shouldn't have any arp entries for devices with IFF_NOARP set,
>> so perhaps we can flush only in that case. The transition IFF_NOARP
>> -> ~IFF_NOARP shouldn't need flushing.
> 
> IFF_NOARP devices do have neighbor entries with the nud NOARP.
> Exactly those entries I want to flush when IFF_NOARP flag is
> removed.
> 
> You can see those entries with "ip neigh show nud all". You have
> them e.g. for loopback stuff and broad-/multicast stuff in general.
> With IFF_NOARP you get them on all unicast addresses used.

I see. I don't have a better suggestion, except perhaps
to store the bit in dev->priv_flags.

Re: [PATCH 1/2] gre: fix hard header destination address checking

[David Miller]

From: Timo Teras <timo.teras@iki.fi>
Date: Wed,  3 Mar 2010 16:01:13 +0200

> ipgre_header() can be called with zero daddr when the gre device is
> configured as multipoint tunnel and still has the NOARP flag set (which is
> typically cleared by the userspace arp daemon).  If the NOARP packets are
> not dropped, ipgre_tunnel_xmit() will take rt->rt_gateway (= NBMA IP) and
> use that for route look up (and may lead to bogus xfrm acquires).
> 
> The multicast address check is removed as sending to multicast group should
> be ok.  In fact, if gre device has a multicast address as destination
> ipgre_header is always called with multicast address.
> 
> Signed-off-by: Timo Teras <timo.teras@iki.fi>

Applied.

Re: [PATCH 2/2] ipv4: flush ARP entries on device change

[Timo Teräs]

Patrick McHardy wrote:
> Timo Teräs wrote:
>> Patrick McHardy wrote:
>>> Timo Teräs wrote:
>>>> Patrick McHardy wrote:
>>>>> Timo Teras wrote:
>>>>> It would be nice if we could restrict this to IFF_NOARP changes.
>>>> Yes. But I did not see any easy way to figure out which flags have
>>>> changed.
>>>>
>>>> Should we just keep a copy of the previous IFF_NOARP bit somewhere
>>>> (where?).
>>>> Or did I miss something obvious?
>>> We shouldn't have any arp entries for devices with IFF_NOARP set,
>>> so perhaps we can flush only in that case. The transition IFF_NOARP
>>> -> ~IFF_NOARP shouldn't need flushing.
>> IFF_NOARP devices do have neighbor entries with the nud NOARP.
>> Exactly those entries I want to flush when IFF_NOARP flag is
>> removed.
>>
>> You can see those entries with "ip neigh show nud all". You have
>> them e.g. for loopback stuff and broad-/multicast stuff in general.
>> With IFF_NOARP you get them on all unicast addresses used.
> 
> I see. I don't have a better suggestion, except perhaps
> to store the bit in dev->priv_flags.

Ok.

Should I make a patch that uses dev->priv_flags and repost?

Or would make sense to have more generic way, e.g. keep copy
of the previous flags in struct net_device so notifiers can
check which ones changed (or would this have locking issues?).

- Timo