| Message ID | b2c5c4c68f0ae715802b4b4fd35fd1031c65aac0.1415108561.git.mleitner@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 11/4/2014 4:46 PM, Marcelo Ricardo Leitner wrote: > Currently, we only match against local port number in order to reuse > socket. But if this new vxlan wants an IPv6 socket and a IPv4 one bound > to that port, vxlan will reuse an IPv4 socket as IPv6 and a panic will > follow. The following steps reproduce it: > # ip link add vxlan6 type vxlan id 42 group 229.10.10.10 \ > srcport 5000 6000 dev eth0 > # ip link add vxlan7 type vxlan id 43 group ff0e::110 \ > srcport 5000 6000 dev eth0 > # ip link set vxlan6 up > # ip link set vxlan7 up > <panic> > [ 4.187481] BUG: unable to handle kernel NULL pointer dereference at 0000000000000058 > [ 4.187509] IP: [<ffffffff81667c98>] ipv6_sock_mc_join+0x88/0x630 > ... > [ 4.188076] Call Trace: > [ 4.188085] [<ffffffff81667c4a>] ? ipv6_sock_mc_join+0x3a/0x630 > [ 4.188098] [<ffffffffa05a6ad6>] vxlan_igmp_join+0x66/0xd0 [vxlan] > [ 4.188113] [<ffffffff810a3430>] process_one_work+0x220/0x710 > [ 4.188125] [<ffffffff810a33c4>] ? process_one_work+0x1b4/0x710 > [ 4.188138] [<ffffffff810a3a3b>] worker_thread+0x11b/0x3a0 > [ 4.188149] [<ffffffff810a3920>] ? process_one_work+0x710/0x710 > So address family must also match in order to reuse a socket. > Reported-by: Jean-Tsung Hsiao <jhsiao@redhat.com> > Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> > --- > drivers/net/vxlan.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > index ca309820d39e1ba7995f38d3a2f9bacbd1c1f857..c0fa76d55ae3cc07fb14b70656d6b13b5bab091c 100644 > --- a/drivers/net/vxlan.c > +++ b/drivers/net/vxlan.c > @@ -281,7 +281,8 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, __be16 port) > struct vxlan_sock *vs; > > hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) { > - if (inet_sk(vs->sock->sk)->inet_sport == port) > + if ((inet_sk(vs->sock->sk)->inet_sport == port) && > + (inet_sk(vs->sock->sk)->sk.sk_family == family)) The continuation line should start below the next character after ( of the *if* statement, according to the networking coding style. And inner () are not necessary. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Marcelo Ricardo Leitner <mleitner@redhat.com> Date: Tue, 4 Nov 2014 11:46:49 -0200 > hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) { > - if (inet_sk(vs->sock->sk)->inet_sport == port) > + if ((inet_sk(vs->sock->sk)->inet_sport == port) && > + (inet_sk(vs->sock->sk)->sk.sk_family == family)) This is not indented properly. For a multi-line conditional, the second and subsequent lines should start precisely at the first column after the openning parenthesis on the first line. You must use the appropriate number of TAB and SPACE characters necessary to achieve this. If you are only using TAB characters, you are most likely doing it wrong. Please fix this up and resubmit, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04-11-2014 14:32, David Miller wrote: > From: Marcelo Ricardo Leitner <mleitner@redhat.com> > Date: Tue, 4 Nov 2014 11:46:49 -0200 > >> hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) { >> - if (inet_sk(vs->sock->sk)->inet_sport == port) >> + if ((inet_sk(vs->sock->sk)->inet_sport == port) && >> + (inet_sk(vs->sock->sk)->sk.sk_family == family)) > > This is not indented properly. > > For a multi-line conditional, the second and subsequent lines > should start precisely at the first column after the openning > parenthesis on the first line. > > You must use the appropriate number of TAB and SPACE characters > necessary to achieve this. If you are only using TAB characters, you > are most likely doing it wrong. Yes.. I forgot to switch profiles, was using tab spacing=4, then it was aligned and I didn't notice it. > Please fix this up and resubmit, thanks. Okay, sorry. Thanks David, Sergei. Marcelo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index ca309820d39e1ba7995f38d3a2f9bacbd1c1f857..c0fa76d55ae3cc07fb14b70656d6b13b5bab091c 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -281,7 +281,8 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, __be16 port) struct vxlan_sock *vs; hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) { - if (inet_sk(vs->sock->sk)->inet_sport == port) + if ((inet_sk(vs->sock->sk)->inet_sport == port) && + (inet_sk(vs->sock->sk)->sk.sk_family == family)) return vs; } return NULL;
Currently, we only match against local port number in order to reuse socket. But if this new vxlan wants an IPv6 socket and a IPv4 one bound to that port, vxlan will reuse an IPv4 socket as IPv6 and a panic will follow. The following steps reproduce it: # ip link add vxlan6 type vxlan id 42 group 229.10.10.10 \ srcport 5000 6000 dev eth0 # ip link add vxlan7 type vxlan id 43 group ff0e::110 \ srcport 5000 6000 dev eth0 # ip link set vxlan6 up # ip link set vxlan7 up <panic> [ 4.187481] BUG: unable to handle kernel NULL pointer dereference at 0000000000000058 [ 4.187509] IP: [<ffffffff81667c98>] ipv6_sock_mc_join+0x88/0x630 ... [ 4.188076] Call Trace: [ 4.188085] [<ffffffff81667c4a>] ? ipv6_sock_mc_join+0x3a/0x630 [ 4.188098] [<ffffffffa05a6ad6>] vxlan_igmp_join+0x66/0xd0 [vxlan] [ 4.188113] [<ffffffff810a3430>] process_one_work+0x220/0x710 [ 4.188125] [<ffffffff810a33c4>] ? process_one_work+0x1b4/0x710 [ 4.188138] [<ffffffff810a3a3b>] worker_thread+0x11b/0x3a0 [ 4.188149] [<ffffffff810a3920>] ? process_one_work+0x710/0x710 So address family must also match in order to reuse a socket. Reported-by: Jean-Tsung Hsiao <jhsiao@redhat.com> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> --- drivers/net/vxlan.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)