[PATCH net] net: vxlan: fix crash when interface is created with no group

[PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

If the vxlan interface is created without group definition, there is a
panic on the first packet reception:

$ ip link add dev vxlan0 type vxlan id 1
$ ip addr add dev vxlan0 10.0.0.1/24
$ ip link set up dev vxlan0

  BUG: unable to handle kernel paging request at 0000000100000103
  IP: [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
  PGD 7c397067 PUD 0
  Oops: 0000 [#1] SMP
  Modules linked in:
  CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc6-hvx-xen-00153-gee7d07e #95
  Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
  task: ffffffff81813450 ti: ffffffff81800000 task.ti: ffffffff81800000
  RIP: 0010:[<ffffffff8143435b>]  [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
  RSP: 0018:ffff88007fc03d78  EFLAGS: 00010282
  RAX: 0000000100000003 RBX: ffff88007bd29000 RCX: 0000000000000000
  RDX: ffff88007bd29028 RSI: ffff88007c29a000 RDI: ffff88007bd29040
  RBP: ffff88007fc03da8 R08: 0000000000000000 R09: ffff88007b1bc548
  R10: ffff88007bd29a00 R11: ffff88007bd29000 R12: ffff88007bcc5800
  R13: ffffffff8186a000 R14: ffff88007c29a000 R15: 0000000000000000
  FS:  0000000000000000(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
  CR2: 0000000100000103 CR3: 000000007bc01000 CR4: 00000000000006f0
  Stack:
   ffff88007bd29a00 ffffffff81886010 ffffffff8187fa48 000000000000dd86
   ffff88007c29a000 0000000000000000 ffff88007fc03e18 ffffffff8139a42c
   ffff88007fc03dd8 ffffffff812a320f ffffffff8187fa70 ffff88007bd29000
  Call Trace:
   <IRQ>
   [<ffffffff8139a42c>] __netif_receive_skb_core+0x43e/0x478
   [<ffffffff812a320f>] ? virtqueue_poll+0x16/0x27
   [<ffffffff8139a4bb>] __netif_receive_skb+0x55/0x5a
   [<ffffffff8139a536>] process_backlog+0x76/0x12f
   [<ffffffff8139a864>] net_rx_action+0xa2/0x1ab
   [<ffffffff81047847>] __do_softirq+0xca/0x1d1
   [<ffffffff81047ace>] irq_exit+0x3e/0x85
   [<ffffffff8100b98b>] do_IRQ+0xa9/0xc4
   [<ffffffff814a972d>] common_interrupt+0x6d/0x6d
   <EOI>
   [<ffffffff810378db>] ? native_safe_halt+0x6/0x8
   [<ffffffff810110c7>] default_idle+0x9/0xd
   [<ffffffff81011694>] arch_cpu_idle+0x13/0x1c
   [<ffffffff810747fd>] cpu_startup_entry+0xbc/0x137
   [<ffffffff8149bd8e>] rest_init+0x72/0x74
   [<ffffffff8189eda7>] start_kernel+0x3e6/0x3f3
   [<ffffffff8189e7ca>] ? repair_env_string+0x56/0x56
   [<ffffffff8189e120>] ? early_idt_handlers+0x120/0x120
   [<ffffffff8189e4cd>] x86_64_start_reservations+0x2a/0x2c
   [<ffffffff8189e5c2>] x86_64_start_kernel+0xf3/0x102
  Code: 40 68 e9 a9 02 00 00 48 8d 53 28 31 c0 b9 06 00 00 00 48 89 d7 f3 ab 48 8b 43 58 48 83 e0 fe 74 12 48 8b 80 48 01 00 00 48 8b 00 <8b> 80 00 01 00 00 eb 07 41 8b 86 00 01 00 00 8b 53 68 89 43 28
  RIP  [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
   RSP <ffff88007fc03d78>
  CR2: 0000000100000103
  ---[ end trace d4e5022768991ebe ]---

The crash occurs because vxlan_rcv decides on protocol version of outer
packed using vxlan->default_dst.remote_ip.sa.sa_family field which is
not initialized if no multicast group was specified at interface
creation time. This causes vxlan driver to always assume that outer
packet is IPv6.

Using IP protocol version from skb instead of default destination
address family fixes the problem.

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---
 drivers/net/vxlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index b0f705c..a810ce4 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1206,7 +1206,7 @@ static void vxlan_rcv(struct vxlan_sock *vs,
 		goto drop;
 
 	/* Re-examine inner Ethernet packet */
-	if (remote_ip->sa.sa_family == AF_INET) {
+	if (ip_hdr(skb)->version == 4) {
 		oip = ip_hdr(skb);
 		saddr.sin.sin_addr.s_addr = oip->saddr;
 		saddr.sa.sa_family = AF_INET;
-- 
1.8.1.5

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Stephen Hemminger]

On Mon, 17 Mar 2014 13:17:30 +0200
Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:

> If the vxlan interface is created without group definition, there is a
> panic on the first packet reception:
> 
> $ ip link add dev vxlan0 type vxlan id 1
> $ ip addr add dev vxlan0 10.0.0.1/24
> $ ip link set up dev vxlan0
> 
>   BUG: unable to handle kernel paging request at 0000000100000103
>   IP: [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
>   PGD 7c397067 PUD 0
>   Oops: 0000 [#1] SMP
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc6-hvx-xen-00153-gee7d07e #95
>   Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
>   task: ffffffff81813450 ti: ffffffff81800000 task.ti: ffffffff81800000
>   RIP: 0010:[<ffffffff8143435b>]  [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
>   RSP: 0018:ffff88007fc03d78  EFLAGS: 00010282
>   RAX: 0000000100000003 RBX: ffff88007bd29000 RCX: 0000000000000000
>   RDX: ffff88007bd29028 RSI: ffff88007c29a000 RDI: ffff88007bd29040
>   RBP: ffff88007fc03da8 R08: 0000000000000000 R09: ffff88007b1bc548
>   R10: ffff88007bd29a00 R11: ffff88007bd29000 R12: ffff88007bcc5800
>   R13: ffffffff8186a000 R14: ffff88007c29a000 R15: 0000000000000000
>   FS:  0000000000000000(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>   CR2: 0000000100000103 CR3: 000000007bc01000 CR4: 00000000000006f0
>   Stack:
>    ffff88007bd29a00 ffffffff81886010 ffffffff8187fa48 000000000000dd86
>    ffff88007c29a000 0000000000000000 ffff88007fc03e18 ffffffff8139a42c
>    ffff88007fc03dd8 ffffffff812a320f ffffffff8187fa70 ffff88007bd29000
>   Call Trace:
>    <IRQ>
>    [<ffffffff8139a42c>] __netif_receive_skb_core+0x43e/0x478
>    [<ffffffff812a320f>] ? virtqueue_poll+0x16/0x27
>    [<ffffffff8139a4bb>] __netif_receive_skb+0x55/0x5a
>    [<ffffffff8139a536>] process_backlog+0x76/0x12f
>    [<ffffffff8139a864>] net_rx_action+0xa2/0x1ab
>    [<ffffffff81047847>] __do_softirq+0xca/0x1d1
>    [<ffffffff81047ace>] irq_exit+0x3e/0x85
>    [<ffffffff8100b98b>] do_IRQ+0xa9/0xc4
>    [<ffffffff814a972d>] common_interrupt+0x6d/0x6d
>    <EOI>
>    [<ffffffff810378db>] ? native_safe_halt+0x6/0x8
>    [<ffffffff810110c7>] default_idle+0x9/0xd
>    [<ffffffff81011694>] arch_cpu_idle+0x13/0x1c
>    [<ffffffff810747fd>] cpu_startup_entry+0xbc/0x137
>    [<ffffffff8149bd8e>] rest_init+0x72/0x74
>    [<ffffffff8189eda7>] start_kernel+0x3e6/0x3f3
>    [<ffffffff8189e7ca>] ? repair_env_string+0x56/0x56
>    [<ffffffff8189e120>] ? early_idt_handlers+0x120/0x120
>    [<ffffffff8189e4cd>] x86_64_start_reservations+0x2a/0x2c
>    [<ffffffff8189e5c2>] x86_64_start_kernel+0xf3/0x102
>   Code: 40 68 e9 a9 02 00 00 48 8d 53 28 31 c0 b9 06 00 00 00 48 89 d7 f3 ab 48 8b 43 58 48 83 e0 fe 74 12 48 8b 80 48 01 00 00 48 8b 00 <8b> 80 00 01 00 00 eb 07 41 8b 86 00 01 00 00 8b 53 68 89 43 28
>   RIP  [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
>    RSP <ffff88007fc03d78>
>   CR2: 0000000100000103
>   ---[ end trace d4e5022768991ebe ]---
> 
> The crash occurs because vxlan_rcv decides on protocol version of outer
> packed using vxlan->default_dst.remote_ip.sa.sa_family field which is
> not initialized if no multicast group was specified at interface
> creation time. This causes vxlan driver to always assume that outer
> packet is IPv6.
> 
> Using IP protocol version from skb instead of default destination
> address family fixes the problem.
> 
> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
> ---
>  drivers/net/vxlan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index b0f705c..a810ce4 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1206,7 +1206,7 @@ static void vxlan_rcv(struct vxlan_sock *vs,
>  		goto drop;
>  
>  	/* Re-examine inner Ethernet packet */
> -	if (remote_ip->sa.sa_family == AF_INET) {
> +	if (ip_hdr(skb)->version == 4) {
>  		oip = ip_hdr(skb);
>  		saddr.sin.sin_addr.s_addr = oip->saddr;
>  		saddr.sa.sa_family = AF_INET;


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Or Gerlitz]

On Mon, Mar 17, 2014 at 1:17 PM, Mike Rapoport
<mike.rapoport@ravellosystems.com> wrote:
> If the vxlan interface is created without group definition, there is a
> panic on the first packet reception:
>
> $ ip link add dev vxlan0 type vxlan id 1
> $ ip addr add dev vxlan0 10.0.0.1/24
> $ ip link set up dev vxlan0
>
>   BUG: unable to handle kernel paging request at 0000000100000103
>   IP: [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399

Hi Mike,

So this bug/fix is for 3.14 and also earlier kernels?

Or.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Tue, Mar 18, 2014 at 5:10 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
> On Mon, Mar 17, 2014 at 1:17 PM, Mike Rapoport
> <mike.rapoport@ravellosystems.com> wrote:
>> If the vxlan interface is created without group definition, there is a
>> panic on the first packet reception:
>>
>> $ ip link add dev vxlan0 type vxlan id 1
>> $ ip addr add dev vxlan0 10.0.0.1/24
>> $ ip link set up dev vxlan0
>>
>>   BUG: unable to handle kernel paging request at 0000000100000103
>>   IP: [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
>
> Hi Mike,
>
> So this bug/fix is for 3.14 and also earlier kernels?

I think the bug was introduced by addition of ipv6 to vxlan, which was
merged in 3.12.

> Or.



-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Cong Wang]

On Mon, Mar 17, 2014 at 4:17 AM, Mike Rapoport
<mike.rapoport@ravellosystems.com> wrote:
...
> The crash occurs because vxlan_rcv decides on protocol version of outer
> packed using vxlan->default_dst.remote_ip.sa.sa_family field which is
> not initialized if no multicast group was specified at interface
> creation time. This causes vxlan driver to always assume that outer
> packet is IPv6.
>
> Using IP protocol version from skb instead of default destination
> address family fixes the problem.
>
> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>

Acked-by: Cong Wang <cwang@twopensource.com>

Thanks for the fix!

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Stevens]

Wouldn't it be better to:

1) make sure all of vxlan_dev is initialized before use,
      especially default_dst
2) change the v6 code to check for AF_INET6 too, and do
      nothing if not set. If not set by the admin, the family of
      default_dst would then be AF_UNSPEC and not match.

                                                     +-DLS

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Cong Wang]

On Tue, Mar 18, 2014 at 9:55 AM, David Stevens <dlstevens@us.ibm.com> wrote:
>
> Wouldn't it be better to:
>
> 1) make sure all of vxlan_dev is initialized before use,
>       especially default_dst
> 2) change the v6 code to check for AF_INET6 too, and do
>       nothing if not set. If not set by the admin, the family of
>       default_dst would then be AF_UNSPEC and not match.
>

Good point! But that needs to change tx path as well, basically
all "if ( sa_family == AF_INET) ... else ...." needs to change
to "if (sa_family == AF_INET) ... else if (==AF_INET6) ... else ...".

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: Mike Rapoport <mike.rapoport@ravellosystems.com>
Date: Tue, 18 Mar 2014 17:51:23 +0200

> On Tue, Mar 18, 2014 at 5:10 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> On Mon, Mar 17, 2014 at 1:17 PM, Mike Rapoport
>> <mike.rapoport@ravellosystems.com> wrote:
>>> If the vxlan interface is created without group definition, there is a
>>> panic on the first packet reception:
>>>
>>> $ ip link add dev vxlan0 type vxlan id 1
>>> $ ip addr add dev vxlan0 10.0.0.1/24
>>> $ ip link set up dev vxlan0
>>>
>>>   BUG: unable to handle kernel paging request at 0000000100000103
>>>   IP: [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
>>
>> Hi Mike,
>>
>> So this bug/fix is for 3.14 and also earlier kernels?
> 
> I think the bug was introduced by addition of ipv6 to vxlan, which was
> merged in 3.12.

How did this code behave before ipv6 support was added?

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Tue, Mar 18, 2014 at 11:20:27PM -0400, David Miller wrote:
> From: Mike Rapoport <mike.rapoport@ravellosystems.com>
> Date: Tue, 18 Mar 2014 17:51:23 +0200
>
> > On Tue, Mar 18, 2014 at 5:10 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
> >> On Mon, Mar 17, 2014 at 1:17 PM, Mike Rapoport
> >> <mike.rapoport@ravellosystems.com> wrote:
> >>> If the vxlan interface is created without group definition, there is a
> >>> panic on the first packet reception:
> >>>
> >>> $ ip link add dev vxlan0 type vxlan id 1
> >>> $ ip addr add dev vxlan0 10.0.0.1/24
> >>> $ ip link set up dev vxlan0
> >>>
> >>>   BUG: unable to handle kernel paging request at 0000000100000103
> >>>   IP: [<ffffffff8143435b>] ipv6_rcv+0xfa/0x399
> >>
> >> Hi Mike,
> >>
> >> So this bug/fix is for 3.14 and also earlier kernels?
> >
> > I think the bug was introduced by addition of ipv6 to vxlan, which was
> > merged in 3.12.
>
> How did this code behave before ipv6 support was added?

With IPv4 only the outer IP header was just ip_hdr(skb).
The relevantange from ipv6 support patch is this:

@@ -917,9 +1053,20 @@ static void vxlan_rcv(struct vxlan_sock *vs,
                goto drop;

        /* Re-examine inner Ethernet packet */
-       oip = ip_hdr(skb);
+       if (remote_ip->sa.sa_family == AF_INET) {
+               oip = ip_hdr(skb);
+               saddr.sin.sin_addr.s_addr = oip->saddr;
+               saddr.sa.sa_family = AF_INET;
+#if IS_ENABLED(CONFIG_IPV6)
+       } else {
+               oip6 = ipv6_hdr(skb);
+               saddr.sin6.sin6_addr = oip6->saddr;
+               saddr.sa.sa_family = AF_INET6;
+#endif
+       }
+

--
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Tue, Mar 18, 2014 at 10:55:16AM -0600, David Stevens wrote:
>
> Wouldn't it be better to:
>
> 1) make sure all of vxlan_dev is initialized before use,
> especially default_dst
> 2) change the v6 code to check for AF_INET6 too, and do
> nothing if not set. If not set by the admin, the family of
> default_dst would then be AF_UNSPEC and not match.

The family of default dst is implicitly initailized to AF_UNSPEC because
if kzalloc :)

I agree that explicit check for AF_INET6 is much better than fallthrough
with simple 'else' clause and I can send a patch that makes the checks
for IPv6 as well as default_dst initialization explicit

However, for the particular case in vxlan_rcv, checking the packet
version seems to me semantically more correct than comparing default_dst
protocol family with AF_INET or AF_INET6.

> +-DLS

--
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Stevens]

-----Mike Rapoport <mike.rapoport@ravellosystems.com> wrote: -----

>To: David Stevens/Beaverton/IBM@IBMUS
>From: Mike Rapoport <mike.rapoport@ravellosystems.com>
,,,
>However, for the particular case in vxlan_rcv, checking the packet
>version seems to me semantically more correct than comparing
>default_dst
>protocol family with AF_INET or AF_INET6.

No, that's saying that the sender should determine whether
you are using IPv4 or IPv6, but when the localhost is the
sender, it'll only be using one of those.

If the default_dst is an IPv4 group address, ordinary VXLAN
wouldn't interoperate with IPv6 because the group is not an IPv6
group, but with your patch, it'll still receive and accept IPv6-encapsulated
packets. It just wouldn't send any, if all the fdb entries are also v4.

I think neither the receive packets nor the default_dst, which may
not be set, is the right way to determine this. vxlan_dev->saddr
makes more sense, but it, too, doesn't have to be set.

I'd suggest:
1) if vxlan_dev->saddr is set, use that. Any packets sent should use
          this saddr, if set, so that determines the underlay address family
          and so also what IP version you'll receive.
2) if vxlan_dev->saddr is not set, and default_dst is a multicast group,
          use that address family. For ordinary VXLAN, the group is used
          by all participants so it can't mix v4 and v6. Also, if saddr is set
          and default_dst is a multicast group, the address family should
          match.
3) if neither of those is set, then it isn't ordinary VXLAN, but rather a
         custom configuration. In that case, we should allow both, by
         default, at least. fdb entries can use both v4 and v6, so we
         should receive both.

We could also (or instead) add flags to explicitly allow or disallow v4 and/or v6,
and then, of course, enforce those flag settings for saddr, default_dst
and fdb entries, too.

                                                                  +-DLS

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Wed, Mar 19, 2014 at 4:08 PM, David Stevens <dlstevens@us.ibm.com> wrote:
>
>
> -----Mike Rapoport <mike.rapoport@ravellosystems.com> wrote: -----
>
>>To: David Stevens/Beaverton/IBM@IBMUS
>>From: Mike Rapoport <mike.rapoport@ravellosystems.com>
> ,,,
>>However, for the particular case in vxlan_rcv, checking the packet
>>version seems to me semantically more correct than comparing
>>default_dst
>>protocol family with AF_INET or AF_INET6.
>
> No, that's saying that the sender should determine whether
> you are using IPv4 or IPv6, but when the localhost is the
> sender, it'll only be using one of those.
>
> If the default_dst is an IPv4 group address, ordinary VXLAN
> wouldn't interoperate with IPv6 because the group is not an IPv6
> group, but with your patch, it'll still receive and accept IPv6-encapsulated
> packets. It just wouldn't send any, if all the fdb entries are also v4.

I'm not really familiar with v4 and v6 interoperability, but the
vxlan_socket_create will create v6 socket only if IPv6 is explicitly
requested.
Would it be possible to receive IPv6-encapsulated packets from IPv4 socket?

> I think neither the receive packets nor the default_dst, which may
> not be set, is the right way to determine this. vxlan_dev->saddr
> makes more sense, but it, too, doesn't have to be set.
>
> I'd suggest:
> 1) if vxlan_dev->saddr is set, use that. Any packets sent should use
>           this saddr, if set, so that determines the underlay address family
>           and so also what IP version you'll receive.
> 2) if vxlan_dev->saddr is not set, and default_dst is a multicast group,
>           use that address family. For ordinary VXLAN, the group is used
>           by all participants so it can't mix v4 and v6. Also, if saddr is set
>           and default_dst is a multicast group, the address family should
>           match.
> 3) if neither of those is set, then it isn't ordinary VXLAN, but rather a
>          custom configuration. In that case, we should allow both, by
>          default, at least. fdb entries can use both v4 and v6, so we
>          should receive both.
>
> We could also (or instead) add flags to explicitly allow or disallow v4 and/or v6,
> and then, of course, enforce those flag settings for saddr, default_dst
> and fdb entries, too.
>
>                                                                   +-DLS
>



-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Stevens]

-----Mike Rapoport <mike.rapoport@ravellosystems.com> wrote: -----

>I'm not really familiar with v4 and v6 interoperability, but the
>vxlan_socket_create will create v6 socket only if IPv6 is explicitly
>requested.
>Would it be possible to receive IPv6-encapsulated packets from IPv4
>socket?

    Yes, if it's bound to INADDR_ANY or "::", which would be the case
when "vxlan_dev->saddr" aka "ip ... local XXXX"  is not specified.

    And it's a good thing, if mixing protocols is what you want, but I
think we need to make sure the admin can choose whether it's v4, v6
or both.

                                                                   +-DLS

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: Mike Rapoport <mike.rapoport@ravellosystems.com>
Date: Wed, 19 Mar 2014 09:14:46 +0200

> On Tue, Mar 18, 2014 at 10:55:16AM -0600, David Stevens wrote:
>>
>> Wouldn't it be better to:
>>
>> 1) make sure all of vxlan_dev is initialized before use,
>> especially default_dst
>> 2) change the v6 code to check for AF_INET6 too, and do
>> nothing if not set. If not set by the admin, the family of
>> default_dst would then be AF_UNSPEC and not match.
> 
> The family of default dst is implicitly initailized to AF_UNSPEC because
> if kzalloc :)
> 
> I agree that explicit check for AF_INET6 is much better than fallthrough
> with simple 'else' clause and I can send a patch that makes the checks
> for IPv6 as well as default_dst initialization explicit
> 
> However, for the particular case in vxlan_rcv, checking the packet
> version seems to me semantically more correct than comparing default_dst
> protocol family with AF_INET or AF_INET6.

The way I read things, we would receive packets unconditionally in the
pre-ipv6-support code.  So we have to keep doing so.

Therefore we either have to check the SKB protocol or pass an explicit
protocol as an argument to vs->rcv(...).

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Wed, Mar 19, 2014 at 9:46 PM, David Miller <davem@davemloft.net> wrote:
> From: Mike Rapoport <mike.rapoport@ravellosystems.com>
> Date: Wed, 19 Mar 2014 09:14:46 +0200
>
>> On Tue, Mar 18, 2014 at 10:55:16AM -0600, David Stevens wrote:
>>>
>>> Wouldn't it be better to:
>>>
>>> 1) make sure all of vxlan_dev is initialized before use,
>>> especially default_dst
>>> 2) change the v6 code to check for AF_INET6 too, and do
>>> nothing if not set. If not set by the admin, the family of
>>> default_dst would then be AF_UNSPEC and not match.
>>
>> The family of default dst is implicitly initailized to AF_UNSPEC because
>> if kzalloc :)
>>
>> I agree that explicit check for AF_INET6 is much better than fallthrough
>> with simple 'else' clause and I can send a patch that makes the checks
>> for IPv6 as well as default_dst initialization explicit
>>
>> However, for the particular case in vxlan_rcv, checking the packet
>> version seems to me semantically more correct than comparing default_dst
>> protocol family with AF_INET or AF_INET6.
>
> The way I read things, we would receive packets unconditionally in the
> pre-ipv6-support code.  So we have to keep doing so.
>
> Therefore we either have to check the SKB protocol or pass an explicit
> protocol as an argument to vs->rcv(...).

Well, the patch I've sent checks for ip_hdr(skb) protocol version...

-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Stevens]

-----David Miller <davem@davemloft.net> wrote: -----

>The way I read things, we would receive packets unconditionally in
>the
>pre-ipv6-support code. So we have to keep doing so.

I never tried it, but as there are IP-version-specific processing (the
whole reason we need to check to support both), I'd expect
that before the v6 support patch, v6-encapsulated packets would have
been dropped, or at least mishandled. We accepted all v4 packets,
because v4 is all that was supported.

I think the biggest risk is that someone who is only using or
caring about v4 will have a security vulnerability because
someone can drop packets on the virtual network via v6--
something likely unexpected on an otherwise v4-only network.

When the default_dst is a v4 multicast, or saddr is set to be
a v4 address, we can't have 2-way communication with other
segments using v6, and similary if they are v6, a v4-endpoint
can't join the v6-multicast group.

I think mixing protocols only makes sense when saddr is not
set at all and when default_dst is not a multicast address.
The other possibilities lead to unexpected problems, and
potential mischief.

                                                     +-DLS

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: Mike Rapoport <mike.rapoport@ravellosystems.com>
Date: Wed, 19 Mar 2014 21:52:31 +0200

> On Wed, Mar 19, 2014 at 9:46 PM, David Miller <davem@davemloft.net> wrote:
>> From: Mike Rapoport <mike.rapoport@ravellosystems.com>
>> Date: Wed, 19 Mar 2014 09:14:46 +0200
>>
>>> On Tue, Mar 18, 2014 at 10:55:16AM -0600, David Stevens wrote:
>>>>
>>>> Wouldn't it be better to:
>>>>
>>>> 1) make sure all of vxlan_dev is initialized before use,
>>>> especially default_dst
>>>> 2) change the v6 code to check for AF_INET6 too, and do
>>>> nothing if not set. If not set by the admin, the family of
>>>> default_dst would then be AF_UNSPEC and not match.
>>>
>>> The family of default dst is implicitly initailized to AF_UNSPEC because
>>> if kzalloc :)
>>>
>>> I agree that explicit check for AF_INET6 is much better than fallthrough
>>> with simple 'else' clause and I can send a patch that makes the checks
>>> for IPv6 as well as default_dst initialization explicit
>>>
>>> However, for the particular case in vxlan_rcv, checking the packet
>>> version seems to me semantically more correct than comparing default_dst
>>> protocol family with AF_INET or AF_INET6.
>>
>> The way I read things, we would receive packets unconditionally in the
>> pre-ipv6-support code.  So we have to keep doing so.
>>
>> Therefore we either have to check the SKB protocol or pass an explicit
>> protocol as an argument to vs->rcv(...).
> 
> Well, the patch I've sent checks for ip_hdr(skb) protocol version...

Yes, I know.

I'm just going to wait and see if anyone else in this thread
has anything more to say.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: David Stevens <dlstevens@us.ibm.com>
Date: Wed, 19 Mar 2014 14:28:11 -0600

> 
> 
> -----David Miller <davem@davemloft.net> wrote: -----
> 
>>The way I read things, we would receive packets unconditionally in
>>the
>>pre-ipv6-support code. So we have to keep doing so.
> 
> I never tried it, but as there are IP-version-specific processing (the
> whole reason we need to check to support both), I'd expect
> that before the v6 support patch, v6-encapsulated packets would have
> been dropped, or at least mishandled. We accepted all v4 packets,
> because v4 is all that was supported.

I'm not suggesting to mix protocols, I'm suggesting that the IPV4
path continues to accept all IPV4 packets.

And we know exactly what protocol it is because the vs->rcv() caller
is going to be ipv4 or ipv6 specific code.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: Mike Rapoport <mike.rapoport@ravellosystems.com>
Date: Mon, 17 Mar 2014 13:17:30 +0200

> If the vxlan interface is created without group definition, there is a
> panic on the first packet reception:
 ...
> The crash occurs because vxlan_rcv decides on protocol version of outer
> packed using vxlan->default_dst.remote_ip.sa.sa_family field which is
> not initialized if no multicast group was specified at interface
> creation time. This causes vxlan driver to always assume that outer
> packet is IPv6.
> 
> Using IP protocol version from skb instead of default destination
> address family fixes the problem.
> 
> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>

Thinking some more, I'd like to propose an alternate version of this fix.

Any objections to this?  I think it maintains the pre-ipv6-support
behavior.  I know there may be some concerns about supporting multiple
families on the same socket, but I'm not so sure the code is able to
support that right now anyways.

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index a7eb3f2..3a23623 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1206,7 +1206,7 @@ static void vxlan_rcv(struct vxlan_sock *vs,
 		goto drop;
 
 	/* Re-examine inner Ethernet packet */
-	if (remote_ip->sa.sa_family == AF_INET) {
+	if (vs->family == AF_INET) {
 		oip = ip_hdr(skb);
 		saddr.sin.sin_addr.s_addr = oip->saddr;
 		saddr.sa.sa_family = AF_INET;
@@ -2409,10 +2409,13 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port,
 
 	INIT_WORK(&vs->del_work, vxlan_del_work);
 
-	if (ipv6)
+	if (ipv6) {
+		vs->family = AF_INET6;
 		sock = create_v6_sock(net, port);
-	else
+	} else {
+		vs->family = AF_INET;
 		sock = create_v4_sock(net, port);
+	}
 	if (IS_ERR(sock)) {
 		kfree(vs);
 		return ERR_CAST(sock);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 5deef1a..6f00731 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -16,6 +16,7 @@ struct vxlan_sock {
 	struct hlist_node hlist;
 	vxlan_rcv_t	 *rcv;
 	void		 *data;
+	__u16		  family;
 	struct work_struct del_work;
 	struct socket	 *sock;
 	struct rcu_head	  rcu;

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Stevens]

>From: David Miller 

>Any objections to this? I think it maintains the pre-ipv6-support
>behavior. I know there may be some concerns about supporting
>multiple
>families on the same socket, but I'm not so sure the code is able to
>support that right now anyways.

I'm ok with the idea of determining the AF from the socket -- mixed
support, if useful, can be added later. But the patch needs to then
check and drop packets encapsulated with the wrong address family.

And it still shouldn't assume !v4 means v6.

[apologies for spacing; doing this from a web browser...]
So, I think we need something like:

     if (vs->family == AF_INET && skb->protocol == ntohs(ETH_P_IP)) {
        ....
     } else if (vs->family == AF_INET6 && skb->protocol == ntohs(ETH_P_IPV6)) {
        ...
     } else
           goto drop


                                                          +-DLS

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Thu, Mar 20, 2014 at 10:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Mike Rapoport <mike.rapoport@ravellosystems.com>
> Date: Mon, 17 Mar 2014 13:17:30 +0200
>
>> If the vxlan interface is created without group definition, there is a
>> panic on the first packet reception:
>  ...
>> The crash occurs because vxlan_rcv decides on protocol version of outer
>> packed using vxlan->default_dst.remote_ip.sa.sa_family field which is
>> not initialized if no multicast group was specified at interface
>> creation time. This causes vxlan driver to always assume that outer
>> packet is IPv6.
>>
>> Using IP protocol version from skb instead of default destination
>> address family fixes the problem.
>>
>> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
>
> Thinking some more, I'd like to propose an alternate version of this fix.
>
> Any objections to this?  I think it maintains the pre-ipv6-support
> behavior.  I know there may be some concerns about supporting multiple
> families on the same socket, but I'm not so sure the code is able to
> support that right now anyways.

No objections from my side either.

> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index a7eb3f2..3a23623 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1206,7 +1206,7 @@ static void vxlan_rcv(struct vxlan_sock *vs,
>                 goto drop;
>
>         /* Re-examine inner Ethernet packet */
> -       if (remote_ip->sa.sa_family == AF_INET) {
> +       if (vs->family == AF_INET) {
>                 oip = ip_hdr(skb);
>                 saddr.sin.sin_addr.s_addr = oip->saddr;
>                 saddr.sa.sa_family = AF_INET;
> @@ -2409,10 +2409,13 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port,
>
>         INIT_WORK(&vs->del_work, vxlan_del_work);
>
> -       if (ipv6)
> +       if (ipv6) {
> +               vs->family = AF_INET6;
>                 sock = create_v6_sock(net, port);
> -       else
> +       } else {
> +               vs->family = AF_INET;
>                 sock = create_v4_sock(net, port);
> +       }
>         if (IS_ERR(sock)) {
>                 kfree(vs);
>                 return ERR_CAST(sock);
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 5deef1a..6f00731 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -16,6 +16,7 @@ struct vxlan_sock {
>         struct hlist_node hlist;
>         vxlan_rcv_t      *rcv;
>         void             *data;
> +       __u16             family;
>         struct work_struct del_work;
>         struct socket    *sock;
>         struct rcu_head   rcu;



-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Thu, Mar 20, 2014 at 10:47 PM, David Stevens <dlstevens@us.ibm.com> wrote:
>
>>From: David Miller
>
>>Any objections to this? I think it maintains the pre-ipv6-support
>>behavior. I know there may be some concerns about supporting
>>multiple
>>families on the same socket, but I'm not so sure the code is able to
>>support that right now anyways.
>
> I'm ok with the idea of determining the AF from the socket -- mixed
> support, if useful, can be added later. But the patch needs to then
> check and drop packets encapsulated with the wrong address family.
>
> And it still shouldn't assume !v4 means v6.
>
> [apologies for spacing; doing this from a web browser...]
> So, I think we need something like:
>
>      if (vs->family == AF_INET && skb->protocol == ntohs(ETH_P_IP)) {
>         ....
>      } else if (vs->family == AF_INET6 && skb->protocol == ntohs(ETH_P_IPV6)) {
>         ...
>      } else
>            goto drop
>

Checking skb->protocol will drop ARP requests. What about using
ip_hdr(skb)->version?

>                                                           +-DLS
>



-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Stevens]

-----Mike Rapoport <mike.rapoport@ravellosystems.com> wrote: -----


>Checking skb->protocol will drop ARP requests. What about using
>ip_hdr(skb)->version?

Mike, ip_hdr() here is the outer packet, so it's got to be a UDP packet--
we just don't know if it's UDP/IP or UDP/IPv6 when it is bound to INADDR_ANY,
since both can be delivered. It could use version in this case, because
both possible protocols have version in the same place, but I think it's
more correct to use the MAC layer protocol rather than relying on the
fact that IPv4 and IPv6 have "version" in the same spot. "It could be ARP"
would be the argument for NOT using the version in places where it really
could be ARP, even though that isn't the case here.

vxlan_rcv() is only called for VXLAN encapsulated packets sent to the bound
UDP port.

So, if "vs->family" holds the one we want to support, we can't just blindly
assume the received packet is IPv4, for example, and start accessing
IPv4 fields, because it could be an IPv6 packet. We have to check the
packet type too. And if it's not the one we bound to, drop it.

That's what the code snippet I outlined is trying to do.

                                         +-DLS

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Fri, Mar 21, 2014 at 1:22 PM, David Stevens <dlstevens@us.ibm.com> wrote:
>
> -----Mike Rapoport <mike.rapoport@ravellosystems.com> wrote: -----
>
> Mike, ip_hdr() here is the outer packet, so it's got to be a UDP packet--
> we just don't know if it's UDP/IP or UDP/IPv6 when it is bound to INADDR_ANY,
> since both can be delivered. It could use version in this case, because
> both possible protocols have version in the same place, but I think it's
> more correct to use the MAC layer protocol rather than relying on the
> fact that IPv4 and IPv6 have "version" in the same spot. "It could be ARP"
> would be the argument for NOT using the version in places where it really
> could be ARP, even though that isn't the case here.
>
> vxlan_rcv() is only called for VXLAN encapsulated packets sent to the bound
> UDP port.
>
> So, if "vs->family" holds the one we want to support, we can't just blindly
> assume the received packet is IPv4, for example, and start accessing
> IPv4 fields, because it could be an IPv6 packet. We have to check the
> packet type too. And if it's not the one we bound to, drop it.
>
> That's what the code snippet I outlined is trying to do.
>

David,

I've tried your snippet with IPv4 and I've got all ARP replies
dropped. And if I enable IPv6 I still get crushes in ipv6_rcv.
It seems to me that at the time vxlan_rcv gets outer IP header, the
SKB contains mixed information of outer and inner packets.
I'll continue to look into it.


                                          +-DLS
>
>
>



-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Fri, Mar 21, 2014 at 05:22:06AM -0600, David Stevens wrote:
> -----Mike Rapoport <mike.rapoport@ravellosystems.com> wrote: -----
>
> >Checking skb->protocol will drop ARP requests. What about using
> >ip_hdr(skb)->version?
>
> Mike, ip_hdr() here is the outer packet, so it's got to be a UDP packet--
> we just don't know if it's UDP/IP or UDP/IPv6 when it is bound to INADDR_ANY,
> since both can be delivered. It could use version in this case, because
> both possible protocols have version in the same place, but I think it's
> more correct to use the MAC layer protocol rather than relying on the
> fact that IPv4 and IPv6 have "version" in the same spot. "It could be ARP"
> would be the argument for NOT using the version in places where it really
> could be ARP, even though that isn't the case here.
>
> vxlan_rcv() is only called for VXLAN encapsulated packets sent to the bound
> UDP port.
>
> So, if "vs->family" holds the one we want to support, we can't just blindly
> assume the received packet is IPv4, for example, and start accessing
> IPv4 fields, because it could be an IPv6 packet. We have to check the
> packet type too. And if it's not the one we bound to, drop it.
>
> That's what the code snippet I outlined is trying to do.
>
>                                          +-DLS
>

I beleive I've groked what's going on in vxlan_udp_encap_recv and
vxlan_rcv. There are actually two unrelated problems:

1) When the vxlan is configured with IPv4 group it crashes when it
starts to receive IPv6 IGMP packets encapsulated into IPv4 vxlan
packets. This happens because when ipv6_rcv handles the inner packet,
the skb->dst still refernces outer IPv4 info. The very old vxlan code
had skb_dst_drop call in vxlan_udp_encap_recv, which was removed when
vxlan was refactored to use iptunnel_pull_header (commit
7ce04758279514ca1d8ebfe322508a4a430fe2c8: "vxlan: Restructure vxlan
receive"). The iptunnel_pull_header called skb_dst_drop until recent
commit 10ddceb22bab11dab10ba645c7df2e4a8e7a5db5 ("ip_tunnel:multicast
process cause panic due to skb->_skb_refdst NULL pointer").
The simplest fix, I think, would be to restore call to skb_dst_drop in
vxlan_udp_encap_recv.

2) When the vxlan is using custom configuration and the vxlan interface
is created without group definition, the vxlan_rcv always takes IPv6
path because the decision is based on default_dst.sa.sa_family which is
AF_UNSPEC in this case. The code snippet proposed by David S. is not
working because by the time vxlan_rcv checks the outer protocol the
skb->protocol is already set to that of the inner packet in
iptunnel_pull_header. So, to have proper check for packet type in
vxlan_rcv we can either check outer IP header version or pass outer
skb->protocol to vxlan_rcv.
I personally favor checking ip_hdr(skb)->version despite David S.
objection above. The version field is not coincidentally at the same
spot for both v4 and v6, and check for version keeps code simpler and
cleaner, IMHO.

Waiting for your comments,

Mike.

--
[1] http://www.spinics.net/lists/netdev/msg276490.html

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Or Gerlitz]

On Sun, Mar 23, 2014 at 11:27 AM, Mike Rapoport
<mike.rapoport@ravellosystems.com> wrote:

> I believe I've groked what's going on in vxlan_udp_encap_recv and
> vxlan_rcv. There are actually two unrelated problems:
>
> 1) When the vxlan is configured with IPv4 group it crashes when it
> starts to receive IPv6 IGMP packets encapsulated into IPv4 vxlan
> packets. This happens because when ipv6_rcv handles the inner packet,
> the skb->dst still refernces outer IPv4 info. The very old vxlan code
> had skb_dst_drop call in vxlan_udp_encap_recv, which was removed when
> vxlan was refactored to use iptunnel_pull_header (commit
> 7ce04758279514ca1d8ebfe322508a4a430fe2c8: "vxlan: Restructure vxlan
> receive"). The iptunnel_pull_header called skb_dst_drop until recent
> commit 10ddceb22bab11dab10ba645c7df2e4a8e7a5db5 ("ip_tunnel:multicast
> process cause panic due to skb->_skb_refdst NULL pointer").
> The simplest fix, I think, would be to restore call to skb_dst_drop in
> vxlan_udp_encap_recv.

Yep, following Mike's suggestion, adding the below call allows things to work,
where trying vxlan without OVS, e.g using

$ ip link add vxlan42 type vxlan id 42 group 239.0.0.42 ttl 10 dev ethN

$ ifconfig vxlan42 192.168.42.54/24 up

over the net tree with 3.14-rc6 and beyond crashes instantly on node A
when node B is
taken up and starts sending, so commit 10ddceb22b indeed introduced a
regression.

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index a7eb3f2..22d7484 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1150,6 +1150,8 @@ static int vxlan_udp_encap_recv(struct sock *sk,
struct sk_buff *skb)
        if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
                goto drop;

+       skb_dst_drop(skb);
+
        port = inet_sk(sk)->inet_sport;

        vs = rcu_dereference_sk_user_data(sk);

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Pravin Shelar]

On Sun, Mar 23, 2014 at 2:27 AM, Mike Rapoport
<mike.rapoport@ravellosystems.com> wrote:
> On Fri, Mar 21, 2014 at 05:22:06AM -0600, David Stevens wrote:
>> -----Mike Rapoport <mike.rapoport@ravellosystems.com> wrote: -----
>>
>> >Checking skb->protocol will drop ARP requests. What about using
>> >ip_hdr(skb)->version?
>>
>> Mike, ip_hdr() here is the outer packet, so it's got to be a UDP packet--
>> we just don't know if it's UDP/IP or UDP/IPv6 when it is bound to INADDR_ANY,
>> since both can be delivered. It could use version in this case, because
>> both possible protocols have version in the same place, but I think it's
>> more correct to use the MAC layer protocol rather than relying on the
>> fact that IPv4 and IPv6 have "version" in the same spot. "It could be ARP"
>> would be the argument for NOT using the version in places where it really
>> could be ARP, even though that isn't the case here.
>>
>> vxlan_rcv() is only called for VXLAN encapsulated packets sent to the bound
>> UDP port.
>>
>> So, if "vs->family" holds the one we want to support, we can't just blindly
>> assume the received packet is IPv4, for example, and start accessing
>> IPv4 fields, because it could be an IPv6 packet. We have to check the
>> packet type too. And if it's not the one we bound to, drop it.
>>
>> That's what the code snippet I outlined is trying to do.
>>
>>                                          +-DLS
>>
>
> I beleive I've groked what's going on in vxlan_udp_encap_recv and
> vxlan_rcv. There are actually two unrelated problems:
>
> 1) When the vxlan is configured with IPv4 group it crashes when it
> starts to receive IPv6 IGMP packets encapsulated into IPv4 vxlan
> packets. This happens because when ipv6_rcv handles the inner packet,
> the skb->dst still refernces outer IPv4 info. The very old vxlan code
> had skb_dst_drop call in vxlan_udp_encap_recv, which was removed when
> vxlan was refactored to use iptunnel_pull_header (commit
> 7ce04758279514ca1d8ebfe322508a4a430fe2c8: "vxlan: Restructure vxlan
> receive"). The iptunnel_pull_header called skb_dst_drop until recent
> commit 10ddceb22bab11dab10ba645c7df2e4a8e7a5db5 ("ip_tunnel:multicast
> process cause panic due to skb->_skb_refdst NULL pointer").
> The simplest fix, I think, would be to restore call to skb_dst_drop in
> vxlan_udp_encap_recv.
>
iptunnel_pull() is used in multiple tunnel implementation, therefore
we need drop dst in that function.
I send out fix : http://marc.info/?l=linux-netdev&m=139563761515707

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: Or Gerlitz <or.gerlitz@gmail.com>
Date: Sun, 23 Mar 2014 16:43:52 +0200

> On Sun, Mar 23, 2014 at 11:27 AM, Mike Rapoport
> <mike.rapoport@ravellosystems.com> wrote:
> 
>> I believe I've groked what's going on in vxlan_udp_encap_recv and
>> vxlan_rcv. There are actually two unrelated problems:
>>
>> 1) When the vxlan is configured with IPv4 group it crashes when it
>> starts to receive IPv6 IGMP packets encapsulated into IPv4 vxlan
>> packets. This happens because when ipv6_rcv handles the inner packet,
>> the skb->dst still refernces outer IPv4 info. The very old vxlan code
>> had skb_dst_drop call in vxlan_udp_encap_recv, which was removed when
>> vxlan was refactored to use iptunnel_pull_header (commit
>> 7ce04758279514ca1d8ebfe322508a4a430fe2c8: "vxlan: Restructure vxlan
>> receive"). The iptunnel_pull_header called skb_dst_drop until recent
>> commit 10ddceb22bab11dab10ba645c7df2e4a8e7a5db5 ("ip_tunnel:multicast
>> process cause panic due to skb->_skb_refdst NULL pointer").
>> The simplest fix, I think, would be to restore call to skb_dst_drop in
>> vxlan_udp_encap_recv.
> 
> Yep, following Mike's suggestion, adding the below call allows things to work,
> where trying vxlan without OVS, e.g using
> 
> $ ip link add vxlan42 type vxlan id 42 group 239.0.0.42 ttl 10 dev ethN
> 
> $ ifconfig vxlan42 192.168.42.54/24 up
> 
> over the net tree with 3.14-rc6 and beyond crashes instantly on node A
> when node B is
> taken up and starts sending, so commit 10ddceb22b indeed introduced a
> regression.

It turns out we are going to partially revert that commit and
iptunnel_pull_header() will start doing the skb_dst_drop() again.

See:

	http://patchwork.ozlabs.org/patch/332956/

for details.

Will that fix this bug too?

Thanks.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Wed, Mar 26, 2014 at 2:53 AM, David Miller <davem@davemloft.net> wrote:
> From: Or Gerlitz <or.gerlitz@gmail.com>
> Date: Sun, 23 Mar 2014 16:43:52 +0200
>
>> On Sun, Mar 23, 2014 at 11:27 AM, Mike Rapoport
>> <mike.rapoport@ravellosystems.com> wrote:
>>
>>> 1) When the vxlan is configured with IPv4 group it crashes when it
>>> starts to receive IPv6 IGMP packets encapsulated into IPv4 vxlan
>>> packets. This happens because when ipv6_rcv handles the inner packet,
>>> the skb->dst still refernces outer IPv4 info. The very old vxlan code
>>> had skb_dst_drop call in vxlan_udp_encap_recv, which was removed when
>>> vxlan was refactored to use iptunnel_pull_header (commit
>>> 7ce04758279514ca1d8ebfe322508a4a430fe2c8: "vxlan: Restructure vxlan
>>> receive"). The iptunnel_pull_header called skb_dst_drop until recent
>>> commit 10ddceb22bab11dab10ba645c7df2e4a8e7a5db5 ("ip_tunnel:multicast
>>> process cause panic due to skb->_skb_refdst NULL pointer").
>>> The simplest fix, I think, would be to restore call to skb_dst_drop in
>>> vxlan_udp_encap_recv.
>>
>> Yep, following Mike's suggestion, adding the below call allows things to work,
>> where trying vxlan without OVS, e.g using
>>
>> $ ip link add vxlan42 type vxlan id 42 group 239.0.0.42 ttl 10 dev ethN
>>
>> $ ifconfig vxlan42 192.168.42.54/24 up
>>
>> over the net tree with 3.14-rc6 and beyond crashes instantly on node A
>> when node B is
>> taken up and starts sending, so commit 10ddceb22b indeed introduced a
>> regression.
>
> It turns out we are going to partially revert that commit and
> iptunnel_pull_header() will start doing the skb_dst_drop() again.
>
> See:
>
>         http://patchwork.ozlabs.org/patch/332956/
>
> for details.
>
> Will that fix this bug too?

It fixes the instant crashes occurring when vxlan interface goes up, but
I have other crashes with custom vxlan configuration, e.g. in the
following scenario:

Node A:
$ ip link add vxlan1 type vxlan id 1 dev ethA
$ ip addr add dev vxlan1 10.0.0.1/24
$ ip link set up dev vxlan1
$ bridge fdb append 00:00:00:00:00:00 dev vxlan1 dst <ethB IPv4 address>

Node B:
$ ip link add vxlan1 type vxlan id 1 dev ethB
$ ip addr add dev vxlan1 10.0.0.2/24
$ ip link set up dev vxlan1
$ bridge fdb append 00:00:00:00:00:00 dev vxlan1 dst <ethA A IPv4 address>
$ ping 10.0.0.1

Node A gets NULL pointer dereference at ip6_route_output.

Setting defailt_dst family to AF_INET (see below) and making it
consistent with actual vxlan socket family solves the problem for IPv4 case. 
As for the IPv6 with custom vxlan configuration, it's not functional at
the moment because unless v6 is explicitly specified by either
IFLA_VXLAN_GROUP6 or IFLA_VXLAN_LOCAL6, the driver assumes IPv4 and will
crash when it's trying to xmit to an fdb entry with IPv6 address.
Disallowing addition of fdb entries with IPv6 destinations in this case
prevents such crashes.
If these changes are acceptable I'll resend them as proper patch.

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 1236812..f389cbc 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -871,6 +871,9 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	if (err)
 		return err;
 
+	if (vxlan->default_dst.remote_ip.sa.sa_family != ip.sa.sa_family)
+		return -EAFNOSUPPORT;
+
 	spin_lock_bh(&vxlan->hash_lock);
 	err = vxlan_fdb_create(vxlan, addr, &ip, ndm->ndm_state, flags,
 			       port, vni, ifindex, ndm->ndm_flags);
@@ -2612,9 +2615,9 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
 	vni = nla_get_u32(data[IFLA_VXLAN_ID]);
 	dst->remote_vni = vni;
 
+	dst->remote_ip.sa.sa_family = AF_INET;
 	if (data[IFLA_VXLAN_GROUP]) {
 		dst->remote_ip.sin.sin_addr.s_addr = nla_get_be32(data[IFLA_VXLAN_GROUP]);
-		dst->remote_ip.sa.sa_family = AF_INET;
 	} else if (data[IFLA_VXLAN_GROUP6]) {
 		if (!IS_ENABLED(CONFIG_IPV6))
 			return -EPFNOSUPPORT;

> Thanks.

-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Stevens]

Mike Rapoport <mike.rapoport@ravellosystems.com> wrote on 03/26/2014 05:47:54 AM:

> 
> It fixes the instant crashes occurring when vxlan interface goes up, but
> I have other crashes with custom vxlan configuration, e.g. in the
> following scenario:
> 
> Node A:
> $ ip link add vxlan1 type vxlan id 1 dev ethA
> $ ip addr add dev vxlan1 10.0.0.1/24
> $ ip link set up dev vxlan1
> $ bridge fdb append 00:00:00:00:00:00 dev vxlan1 dst <ethB IPv4 address>

I thought this MAC address would be an error, but I just noticed that you
submitted a patch adding interpretation of the all zeroes MAC address as
a default FDB.

That patch, IMO, should've either not gone in, or should've replaced (entirely)
the existing default_dst. I think the problems you are seeing are due to having
two "defaults".

Since the existing default_dst could be either a multicast group or a specific
destination, I don't understand the point of your patch:
    afbd8bae9c798c5cdbe4439d3a50536b5438247c

except possibly to support it as a list. In that case, at least that patch should've
removed default_dst and used the all-zeroes fdb entry instead, including group
joins and leaves when it is a multicast IP address.

I certainly don't see the point of having both.

                                                                      +-DLS

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Wed, Mar 26, 2014 at 08:47:19AM -0600, David Stevens wrote:
> Mike Rapoport <mike.rapoport@ravellosystems.com> wrote on 03/26/2014 05:47:54 AM:
> 
> > 
> > It fixes the instant crashes occurring when vxlan interface goes up, but
> > I have other crashes with custom vxlan configuration, e.g. in the
> > following scenario:
> > 
> > Node A:
> > $ ip link add vxlan1 type vxlan id 1 dev ethA
> > $ ip addr add dev vxlan1 10.0.0.1/24
> > $ ip link set up dev vxlan1
> > $ bridge fdb append 00:00:00:00:00:00 dev vxlan1 dst <ethB IPv4 address>
> 
> I thought this MAC address would be an error, but I just noticed that you
> submitted a patch adding interpretation of the all zeroes MAC address as
> a default FDB.
> 
> That patch, IMO, should've either not gone in, or should've replaced (entirely)
> the existing default_dst. I think the problems you are seeing are due to having
> two "defaults".
> 
> Since the existing default_dst could be either a multicast group or a specific
> destination, I don't understand the point of your patch:
>     afbd8bae9c798c5cdbe4439d3a50536b5438247c
> 
> except possibly to support it as a list. In that case, at least that patch should've
> removed default_dst and used the all-zeroes fdb entry instead, including group
> joins and leaves when it is a multicast IP address.
> 
> I certainly don't see the point of having both.
 
The idea was to have a list of multiple destinations allow a weird form
of multicast when infrastructure does not support it.

I don't remember now why I didn't follow your advice about removing
default_dst then (1). Maybe I had some thoughts that seemed smart and
maybe it was just lazines.
And now I definitely regret :)

The problem is not only duplication of default destinations, but also
the mis^Wuse of default_dst to distinguish whether IPv4 or IPv6 path
should be taken because there is no explicit specification of the
protocol if neither IFLA_VXLAN_GROUP or IFLA_VXLAN_LOCAL defined at
vxlan_newlink. Moreover, if neither of these attributes is present, the
custom configuration will always use v4 socket regardles of use of
default_dst or fdb entry.

>                                                     +-DLS
> 

--
[1] http://thread.gmane.org/gmane.linux.network/270969/focus=271830

--
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Cong Wang]

On Wed, Mar 26, 2014 at 10:50 AM, Mike Rapoport
<mike.rapoport@ravellosystems.com> wrote:
>
> The problem is not only duplication of default destinations, but also
> the mis^Wuse of default_dst to distinguish whether IPv4 or IPv6 path
> should be taken because there is no explicit specification of the
> protocol if neither IFLA_VXLAN_GROUP or IFLA_VXLAN_LOCAL defined at
> vxlan_newlink. Moreover, if neither of these attributes is present, the
> custom configuration will always use v4 socket regardles of use of
> default_dst or fdb entry.

It should default to IPv4 for compatibility reason.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Thu, Mar 27, 2014 at 01:20:25PM -0700, Cong Wang wrote:
> On Wed, Mar 26, 2014 at 10:50 AM, Mike Rapoport
> <mike.rapoport@ravellosystems.com> wrote:
> >
> > The problem is not only duplication of default destinations, but also
> > the mis^Wuse of default_dst to distinguish whether IPv4 or IPv6 path
> > should be taken because there is no explicit specification of the
> > protocol if neither IFLA_VXLAN_GROUP or IFLA_VXLAN_LOCAL defined at
> > vxlan_newlink. Moreover, if neither of these attributes is present, the
> > custom configuration will always use v4 socket regardles of use of
> > default_dst or fdb entry.
> 
> It should default to IPv4 for compatibility reason.

Yes, of course. The problem is that when IPv4 is not explicitly defined
using GROUP/LOCAL attributes, there are a lot of places that take IPv6
path.
So, what I'm proposing is to explicitly set default_dst family to
AF_INET and override it with AF_INET6 only when IPv6 is explicitly
requested.

--
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

Hi David,

On Tue, Mar 25, 2014 at 08:53:24PM -0400, David Miller wrote:
> 
> It turns out we are going to partially revert that commit and
> iptunnel_pull_header() will start doing the skb_dst_drop() again.
> 
> See:
> 
> 	http://patchwork.ozlabs.org/patch/332956/
> 
> for details.
> 
> Will that fix this bug too?

I've found some more corner cases in the vxlan driver and the patch
below fixes them and my custom configuration case as well.

> Thanks.


>From 455a1b42e12695e9b6860c8a337a1497374870bd Mon Sep 17 00:00:00 2001
From: Mike Rapoport <mike.rapoport@ravellosystems.com>
Date: Sat, 29 Mar 2014 10:43:58 +0300
Subject: [PATCH] net: vxlan: fix crash when interface is created with no group

If the vxlan interface is created without explicit group definition,
there are corner cases which may cause kernel panic.

For instance, in the following scenario:

node A:
$ ip link add dev vxlan42  address 2c:c2:60:00:10:20 type vxlan id 42
$ ip addr add dev vxlan42 10.0.0.1/24
$ ip link set up dev vxlan42
$ arp -i vxlan42 -s 10.0.0.2 2c:c2:60:00:01:02
$ bridge fdb add dev vxlan42 to 2c:c2:60:00:01:02 dst <IPv4 address>
$ ping 10.0.0.2

node B:
$ ip link add dev vxlan42 address 2c:c2:60:00:01:02 type vxlan id 42
$ ip addr add dev vxlan42 10.0.0.2/24
$ ip link set up dev vxlan42
$ arp -i vxlan42 -s 10.0.0.1 2c:c2:60:00:10:20

node B crashes:

 vxlan42: 2c:c2:60:00:10:20 migrated from 4011:eca4:c0a8:6466:c0a8:6415:8e09:2118 to (invalid address)
 vxlan42: 2c:c2:60:00:10:20 migrated from 4011:eca4:c0a8:6466:c0a8:6415:8e09:2118 to (invalid address)
 BUG: unable to handle kernel NULL pointer dereference at 0000000000000046
 IP: [<ffffffff8143c459>] ip6_route_output+0x58/0x82
 PGD 7bd89067 PUD 7bd4e067 PMD 0
 Oops: 0000 [#1] SMP
 Modules linked in:
 CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.14.0-rc8-hvx-xen-00019-g97a5221-dirty #154
 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
 task: ffff88007c774f50 ti: ffff88007c79c000 task.ti: ffff88007c79c000
 RIP: 0010:[<ffffffff8143c459>]  [<ffffffff8143c459>] ip6_route_output+0x58/0x82
 RSP: 0018:ffff88007fd03668  EFLAGS: 00010282
 RAX: 0000000000000000 RBX: ffffffff8186a000 RCX: 0000000000000040
 RDX: 0000000000000000 RSI: ffff88007b0e4a80 RDI: ffff88007fd03754
 RBP: ffff88007fd03688 R08: ffff88007b0e4a80 R09: 0000000000000000
 R10: 0200000a0100000a R11: 0001002200000000 R12: ffff88007fd03740
 R13: ffff88007b0e4a80 R14: ffff88007b0e4a80 R15: ffff88007bba0c50
 FS:  0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: 0000000000000046 CR3: 000000007bb60000 CR4: 00000000000006e0
 Stack:
  0000000000000000 ffff88007fd037a0 ffffffff8186a000 ffff88007fd03740
  ffff88007fd036c8 ffffffff814320bb 0000000000006e49 ffff88007b8b7360
  ffff88007bdbf200 ffff88007bcbc000 ffff88007b8b7000 ffff88007b8b7360
 Call Trace:
  <IRQ>
  [<ffffffff814320bb>] ip6_dst_lookup_tail+0x2d/0xa4
  [<ffffffff814322a5>] ip6_dst_lookup+0x10/0x12
  [<ffffffff81323b4e>] vxlan_xmit_one+0x32a/0x68c
  [<ffffffff814a325a>] ? _raw_spin_unlock_irqrestore+0x12/0x14
  [<ffffffff8104c551>] ? lock_timer_base.isra.23+0x26/0x4b
  [<ffffffff8132451a>] vxlan_xmit+0x66a/0x6a8
  [<ffffffff8141a365>] ? ipt_do_table+0x35f/0x37e
  [<ffffffff81204ba2>] ? selinux_ip_postroute+0x41/0x26e
  [<ffffffff8139d0c1>] dev_hard_start_xmit+0x2ce/0x3ce
  [<ffffffff8139d491>] __dev_queue_xmit+0x2d0/0x392
  [<ffffffff813b380f>] ? eth_header+0x28/0xb5
  [<ffffffff8139d569>] dev_queue_xmit+0xb/0xd
  [<ffffffff813a5aa6>] neigh_resolve_output+0x134/0x152
  [<ffffffff813db741>] ip_finish_output2+0x236/0x299
  [<ffffffff813dc074>] ip_finish_output+0x98/0x9d
  [<ffffffff813dc749>] ip_output+0x62/0x67
  [<ffffffff813da9f2>] dst_output+0xf/0x11
  [<ffffffff813dc11c>] ip_local_out+0x1b/0x1f
  [<ffffffff813dcf1b>] ip_send_skb+0x11/0x37
  [<ffffffff813dcf70>] ip_push_pending_frames+0x2f/0x33
  [<ffffffff813ff732>] icmp_push_reply+0x106/0x115
  [<ffffffff813ff9e4>] icmp_reply+0x142/0x164
  [<ffffffff813ffb3b>] icmp_echo.part.16+0x46/0x48
  [<ffffffff813c1d30>] ? nf_iterate+0x43/0x80
  [<ffffffff813d8037>] ? xfrm4_policy_check.constprop.11+0x52/0x52
  [<ffffffff813ffb62>] icmp_echo+0x25/0x27
  [<ffffffff814005f7>] icmp_rcv+0x1d2/0x20a
  [<ffffffff813d8037>] ? xfrm4_policy_check.constprop.11+0x52/0x52
  [<ffffffff813d810d>] ip_local_deliver_finish+0xd6/0x14f
  [<ffffffff813d8037>] ? xfrm4_policy_check.constprop.11+0x52/0x52
  [<ffffffff813d7fde>] NF_HOOK.constprop.10+0x4c/0x53
  [<ffffffff813d82bf>] ip_local_deliver+0x4a/0x4f
  [<ffffffff813d7f7b>] ip_rcv_finish+0x253/0x26a
  [<ffffffff813d7d28>] ? inet_add_protocol+0x3e/0x3e
  [<ffffffff813d7fde>] NF_HOOK.constprop.10+0x4c/0x53
  [<ffffffff813d856a>] ip_rcv+0x2a6/0x2ec
  [<ffffffff8139a9a0>] __netif_receive_skb_core+0x43e/0x478
  [<ffffffff812a346f>] ? virtqueue_poll+0x16/0x27
  [<ffffffff8139aa2f>] __netif_receive_skb+0x55/0x5a
  [<ffffffff8139aaaa>] process_backlog+0x76/0x12f
  [<ffffffff8139add8>] net_rx_action+0xa2/0x1ab
  [<ffffffff81047847>] __do_softirq+0xca/0x1d1
  [<ffffffff81047ace>] irq_exit+0x3e/0x85
  [<ffffffff8100b98b>] do_IRQ+0xa9/0xc4
  [<ffffffff814a37ad>] common_interrupt+0x6d/0x6d
  <EOI>
  [<ffffffff810378db>] ? native_safe_halt+0x6/0x8
  [<ffffffff810110c7>] default_idle+0x9/0xd
  [<ffffffff81011694>] arch_cpu_idle+0x13/0x1c
  [<ffffffff8107480d>] cpu_startup_entry+0xbc/0x137
  [<ffffffff8102e741>] start_secondary+0x1a0/0x1a5
 Code: 24 14 e8 f1 e5 01 00 31 d2 a8 32 0f 95 c2 49 8b 44 24 2c 49 0b 44 24 24 74 05 83 ca 04 eb 1c 4d 85 ed 74 17 49 8b 85 a8 02 00 00 <66> 8b 40 46 66 c1 e8 07 83 e0 07 c1 e0 03 09 c2 4c 89 e6 48 89
 RIP  [<ffffffff8143c459>] ip6_route_output+0x58/0x82
  RSP <ffff88007fd03668>
 CR2: 0000000000000046
 ---[ end trace 4612329caab37efd ]---

When vxlan interface is created without explicit group definition, the
default_dst protocol family is initialiazed to AF_UNSPEC and the driver
assumes IPv4 configuration. On the other side, the default_dst protocol
family is used to differentiate between IPv4 and IPv6 cases and, since,
AF_UNSPEC != AF_INET, the processing takes the IPv6 path.

Making the IPv4 assumption explicit by settting default_dst protocol
family to AF_INET4 and preventing mixing of IPv4 and IPv6 addresses in
snooped fdb entries fixes the corner case crashes.

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---
 drivers/net/vxlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 1236812..d091e52 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -871,6 +871,9 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	if (err)
 		return err;
 
+	if (vxlan->default_dst.remote_ip.sa.sa_family != ip.sa.sa_family)
+		return -EAFNOSUPPORT;
+
 	spin_lock_bh(&vxlan->hash_lock);
 	err = vxlan_fdb_create(vxlan, addr, &ip, ndm->ndm_state, flags,
 			       port, vni, ifindex, ndm->ndm_flags);
@@ -2612,9 +2615,10 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
 	vni = nla_get_u32(data[IFLA_VXLAN_ID]);
 	dst->remote_vni = vni;
 
+	/* Unless IPv6 is explicitly requested, assume IPv4 */
+	dst->remote_ip.sa.sa_family = AF_INET;
 	if (data[IFLA_VXLAN_GROUP]) {
 		dst->remote_ip.sin.sin_addr.s_addr = nla_get_be32(data[IFLA_VXLAN_GROUP]);
-		dst->remote_ip.sa.sa_family = AF_INET;
 	} else if (data[IFLA_VXLAN_GROUP6]) {
 		if (!IS_ENABLED(CONFIG_IPV6))
 			return -EPFNOSUPPORT;
-- 
1.8.1.5

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: Mike Rapoport <mike.rapoport@ravellosystems.com>
Date: Sat, 29 Mar 2014 11:29:18 +0300

> Hi David,
> 
> On Tue, Mar 25, 2014 at 08:53:24PM -0400, David Miller wrote:
>> 
>> It turns out we are going to partially revert that commit and
>> iptunnel_pull_header() will start doing the skb_dst_drop() again.
>> 
>> See:
>> 
>> 	http://patchwork.ozlabs.org/patch/332956/
>> 
>> for details.
>> 
>> Will that fix this bug too?
> 
> I've found some more corner cases in the vxlan driver and the patch
> below fixes them and my custom configuration case as well.

Mike, please submit this formally for inclusion, in a fresh patch posting,
without any other unrelated context, thanks.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[David Miller]

From: Mike Rapoport <mike.rapoport@ravellosystems.com>
Date: Tue,  1 Apr 2014 09:23:01 +0300

> If the vxlan interface is created without explicit group definition,
> there are corner cases which may cause kernel panic.
> 
> For instance, in the following scenario:
 ...
> When vxlan interface is created without explicit group definition, the
> default_dst protocol family is initialiazed to AF_UNSPEC and the driver
> assumes IPv4 configuration. On the other side, the default_dst protocol
> family is used to differentiate between IPv4 and IPv6 cases and, since,
> AF_UNSPEC != AF_INET, the processing takes the IPv6 path.
> 
> Making the IPv4 assumption explicit by settting default_dst protocol
> family to AF_INET4 and preventing mixing of IPv4 and IPv6 addresses in
> snooped fdb entries fixes the corner case crashes.
> 
> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
> ---
> 
> This can happen starting from 3.12 when IPv6 support was merged to vxlan.

Looks good to me, thanks for following up on this.

Applied and queued up for -stable.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

On Tue, Apr 1, 2014 at 10:22 PM, Cong Wang <cwang@twopensource.com> wrote:
> On Mon, Mar 31, 2014 at 11:23 PM, Mike Rapoport
> <mike.rapoport@ravellosystems.com> wrote:
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 1236812..d091e52 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -871,6 +871,9 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>>         if (err)
>>                 return err;
>>
>> +       if (vxlan->default_dst.remote_ip.sa.sa_family != ip.sa.sa_family)
>> +               return -EAFNOSUPPORT;
>> +
>
> Or move this check into vxlan_fdb_parse() so that vxlan_fdb_delete()
> could also catch this error?

The vxlan_fdb_delete will fail to find an entry with address from
incompatible family, and will return -ENOENT.

-- 
Sincerely yours,
Mike.

Re: [PATCH net] net: vxlan: fix crash when interface is created with no group

[Cong Wang]

On Mon, Mar 31, 2014 at 11:23 PM, Mike Rapoport
<mike.rapoport@ravellosystems.com> wrote:
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 1236812..d091e52 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -871,6 +871,9 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>         if (err)
>                 return err;
>
> +       if (vxlan->default_dst.remote_ip.sa.sa_family != ip.sa.sa_family)
> +               return -EAFNOSUPPORT;
> +

Or move this check into vxlan_fdb_parse() so that vxlan_fdb_delete()
could also catch this error?

[PATCH net] net: vxlan: fix crash when interface is created with no group

[Mike Rapoport]

If the vxlan interface is created without explicit group definition,
there are corner cases which may cause kernel panic.

For instance, in the following scenario:

node A:
$ ip link add dev vxlan42  address 2c:c2:60:00:10:20 type vxlan id 42
$ ip addr add dev vxlan42 10.0.0.1/24
$ ip link set up dev vxlan42
$ arp -i vxlan42 -s 10.0.0.2 2c:c2:60:00:01:02
$ bridge fdb add dev vxlan42 to 2c:c2:60:00:01:02 dst <IPv4 address>
$ ping 10.0.0.2

node B:
$ ip link add dev vxlan42 address 2c:c2:60:00:01:02 type vxlan id 42
$ ip addr add dev vxlan42 10.0.0.2/24
$ ip link set up dev vxlan42
$ arp -i vxlan42 -s 10.0.0.1 2c:c2:60:00:10:20

node B crashes:

 vxlan42: 2c:c2:60:00:10:20 migrated from 4011:eca4:c0a8:6466:c0a8:6415:8e09:2118 to (invalid address)
 vxlan42: 2c:c2:60:00:10:20 migrated from 4011:eca4:c0a8:6466:c0a8:6415:8e09:2118 to (invalid address)
 BUG: unable to handle kernel NULL pointer dereference at 0000000000000046
 IP: [<ffffffff8143c459>] ip6_route_output+0x58/0x82
 PGD 7bd89067 PUD 7bd4e067 PMD 0
 Oops: 0000 [#1] SMP
 Modules linked in:
 CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.14.0-rc8-hvx-xen-00019-g97a5221-dirty #154
 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
 task: ffff88007c774f50 ti: ffff88007c79c000 task.ti: ffff88007c79c000
 RIP: 0010:[<ffffffff8143c459>]  [<ffffffff8143c459>] ip6_route_output+0x58/0x82
 RSP: 0018:ffff88007fd03668  EFLAGS: 00010282
 RAX: 0000000000000000 RBX: ffffffff8186a000 RCX: 0000000000000040
 RDX: 0000000000000000 RSI: ffff88007b0e4a80 RDI: ffff88007fd03754
 RBP: ffff88007fd03688 R08: ffff88007b0e4a80 R09: 0000000000000000
 R10: 0200000a0100000a R11: 0001002200000000 R12: ffff88007fd03740
 R13: ffff88007b0e4a80 R14: ffff88007b0e4a80 R15: ffff88007bba0c50
 FS:  0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: 0000000000000046 CR3: 000000007bb60000 CR4: 00000000000006e0
 Stack:
  0000000000000000 ffff88007fd037a0 ffffffff8186a000 ffff88007fd03740
  ffff88007fd036c8 ffffffff814320bb 0000000000006e49 ffff88007b8b7360
  ffff88007bdbf200 ffff88007bcbc000 ffff88007b8b7000 ffff88007b8b7360
 Call Trace:
  <IRQ>
  [<ffffffff814320bb>] ip6_dst_lookup_tail+0x2d/0xa4
  [<ffffffff814322a5>] ip6_dst_lookup+0x10/0x12
  [<ffffffff81323b4e>] vxlan_xmit_one+0x32a/0x68c
  [<ffffffff814a325a>] ? _raw_spin_unlock_irqrestore+0x12/0x14
  [<ffffffff8104c551>] ? lock_timer_base.isra.23+0x26/0x4b
  [<ffffffff8132451a>] vxlan_xmit+0x66a/0x6a8
  [<ffffffff8141a365>] ? ipt_do_table+0x35f/0x37e
  [<ffffffff81204ba2>] ? selinux_ip_postroute+0x41/0x26e
  [<ffffffff8139d0c1>] dev_hard_start_xmit+0x2ce/0x3ce
  [<ffffffff8139d491>] __dev_queue_xmit+0x2d0/0x392
  [<ffffffff813b380f>] ? eth_header+0x28/0xb5
  [<ffffffff8139d569>] dev_queue_xmit+0xb/0xd
  [<ffffffff813a5aa6>] neigh_resolve_output+0x134/0x152
  [<ffffffff813db741>] ip_finish_output2+0x236/0x299
  [<ffffffff813dc074>] ip_finish_output+0x98/0x9d
  [<ffffffff813dc749>] ip_output+0x62/0x67
  [<ffffffff813da9f2>] dst_output+0xf/0x11
  [<ffffffff813dc11c>] ip_local_out+0x1b/0x1f
  [<ffffffff813dcf1b>] ip_send_skb+0x11/0x37
  [<ffffffff813dcf70>] ip_push_pending_frames+0x2f/0x33
  [<ffffffff813ff732>] icmp_push_reply+0x106/0x115
  [<ffffffff813ff9e4>] icmp_reply+0x142/0x164
  [<ffffffff813ffb3b>] icmp_echo.part.16+0x46/0x48
  [<ffffffff813c1d30>] ? nf_iterate+0x43/0x80
  [<ffffffff813d8037>] ? xfrm4_policy_check.constprop.11+0x52/0x52
  [<ffffffff813ffb62>] icmp_echo+0x25/0x27
  [<ffffffff814005f7>] icmp_rcv+0x1d2/0x20a
  [<ffffffff813d8037>] ? xfrm4_policy_check.constprop.11+0x52/0x52
  [<ffffffff813d810d>] ip_local_deliver_finish+0xd6/0x14f
  [<ffffffff813d8037>] ? xfrm4_policy_check.constprop.11+0x52/0x52
  [<ffffffff813d7fde>] NF_HOOK.constprop.10+0x4c/0x53
  [<ffffffff813d82bf>] ip_local_deliver+0x4a/0x4f
  [<ffffffff813d7f7b>] ip_rcv_finish+0x253/0x26a
  [<ffffffff813d7d28>] ? inet_add_protocol+0x3e/0x3e
  [<ffffffff813d7fde>] NF_HOOK.constprop.10+0x4c/0x53
  [<ffffffff813d856a>] ip_rcv+0x2a6/0x2ec
  [<ffffffff8139a9a0>] __netif_receive_skb_core+0x43e/0x478
  [<ffffffff812a346f>] ? virtqueue_poll+0x16/0x27
  [<ffffffff8139aa2f>] __netif_receive_skb+0x55/0x5a
  [<ffffffff8139aaaa>] process_backlog+0x76/0x12f
  [<ffffffff8139add8>] net_rx_action+0xa2/0x1ab
  [<ffffffff81047847>] __do_softirq+0xca/0x1d1
  [<ffffffff81047ace>] irq_exit+0x3e/0x85
  [<ffffffff8100b98b>] do_IRQ+0xa9/0xc4
  [<ffffffff814a37ad>] common_interrupt+0x6d/0x6d
  <EOI>
  [<ffffffff810378db>] ? native_safe_halt+0x6/0x8
  [<ffffffff810110c7>] default_idle+0x9/0xd
  [<ffffffff81011694>] arch_cpu_idle+0x13/0x1c
  [<ffffffff8107480d>] cpu_startup_entry+0xbc/0x137
  [<ffffffff8102e741>] start_secondary+0x1a0/0x1a5
 Code: 24 14 e8 f1 e5 01 00 31 d2 a8 32 0f 95 c2 49 8b 44 24 2c 49 0b 44 24 24 74 05 83 ca 04 eb 1c 4d 85 ed 74 17 49 8b 85 a8 02 00 00 <66> 8b 40 46 66 c1 e8 07 83 e0 07 c1 e0 03 09 c2 4c 89 e6 48 89
 RIP  [<ffffffff8143c459>] ip6_route_output+0x58/0x82
  RSP <ffff88007fd03668>
 CR2: 0000000000000046
 ---[ end trace 4612329caab37efd ]---

When vxlan interface is created without explicit group definition, the
default_dst protocol family is initialiazed to AF_UNSPEC and the driver
assumes IPv4 configuration. On the other side, the default_dst protocol
family is used to differentiate between IPv4 and IPv6 cases and, since,
AF_UNSPEC != AF_INET, the processing takes the IPv6 path.

Making the IPv4 assumption explicit by settting default_dst protocol
family to AF_INET4 and preventing mixing of IPv4 and IPv6 addresses in
snooped fdb entries fixes the corner case crashes.

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---

This can happen starting from 3.12 when IPv6 support was merged to vxlan.

 drivers/net/vxlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 1236812..d091e52 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -871,6 +871,9 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	if (err)
 		return err;
 
+	if (vxlan->default_dst.remote_ip.sa.sa_family != ip.sa.sa_family)
+		return -EAFNOSUPPORT;
+
 	spin_lock_bh(&vxlan->hash_lock);
 	err = vxlan_fdb_create(vxlan, addr, &ip, ndm->ndm_state, flags,
 			       port, vni, ifindex, ndm->ndm_flags);
@@ -2612,9 +2615,10 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
 	vni = nla_get_u32(data[IFLA_VXLAN_ID]);
 	dst->remote_vni = vni;
 
+	/* Unless IPv6 is explicitly requested, assume IPv4 */
+	dst->remote_ip.sa.sa_family = AF_INET;
 	if (data[IFLA_VXLAN_GROUP]) {
 		dst->remote_ip.sin.sin_addr.s_addr = nla_get_be32(data[IFLA_VXLAN_GROUP]);
-		dst->remote_ip.sa.sa_family = AF_INET;
 	} else if (data[IFLA_VXLAN_GROUP6]) {
 		if (!IS_ENABLED(CONFIG_IPV6))
 			return -EPFNOSUPPORT;
-- 
1.8.1.5