| Message ID | 20160902183950.p3iv25jnuwmq74sg@codemonkey.org.uk |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Sep 02, 2016 at 02:39:50PM -0400, Dave Jones wrote: > Neither the failure or success paths of ping_v6_sendmsg release > the dst it acquires. This leads to a flood of warnings from > "net/core/dst.c:288 dst_release" on older kernels that > don't have 8bf4ada2e21378816b28205427ee6b0e1ca4c5f1 backported. > > That patch optimistically hoped this had been fixed post 3.10, but > it seems at least one case wasn't, where I've seen this triggered > a lot from machines doing unprivileged icmp sockets. > > Cc: Martin Lau <kafai@fb.com> > Signed-off-by: Dave Jones <davej@codemonkey.org.uk> > > diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c > index 0900352c924c..0e983b694ee8 100644 > --- a/net/ipv6/ping.c > +++ b/net/ipv6/ping.c > @@ -126,8 +126,10 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > rt = (struct rt6_info *) dst; > > np = inet6_sk(sk); > - if (!np) > - return -EBADF; > + if (!np) { > + err = -EBADF; > + goto dst_err_out; > + } > > if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr)) > fl6.flowi6_oif = np->mcast_oif; > @@ -163,6 +165,9 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > } > release_sock(sk); > > +dst_err_out: > + dst_release(dst); > + > if (err) > return err; > Acked-by: Martin KaFai Lau <kafai@fb.com>
On Tue, 2016-09-06 at 10:36 -0700, Martin KaFai Lau wrote: > On Fri, Sep 02, 2016 at 02:39:50PM -0400, Dave Jones wrote: > > Neither the failure or success paths of ping_v6_sendmsg release > > the dst it acquires. This leads to a flood of warnings from > > "net/core/dst.c:288 dst_release" on older kernels that > > don't have 8bf4ada2e21378816b28205427ee6b0e1ca4c5f1 backported. > > > > That patch optimistically hoped this had been fixed post 3.10, but > > it seems at least one case wasn't, where I've seen this triggered > > a lot from machines doing unprivileged icmp sockets. > > > > Cc: Martin Lau <kafai@fb.com> > > Signed-off-by: Dave Jones <davej@codemonkey.org.uk> > > > > diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c > > index 0900352c924c..0e983b694ee8 100644 > > --- a/net/ipv6/ping.c > > +++ b/net/ipv6/ping.c > > @@ -126,8 +126,10 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > > rt = (struct rt6_info *) dst; > > > > np = inet6_sk(sk); > > - if (!np) > > - return -EBADF; > > + if (!np) { > > + err = -EBADF; > > + goto dst_err_out; > > + } > > > > if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr)) > > fl6.flowi6_oif = np->mcast_oif; > > @@ -163,6 +165,9 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > > } > > release_sock(sk); > > > > +dst_err_out: > > + dst_release(dst); > > + > > if (err) > > return err; > > > > Acked-by: Martin KaFai Lau <kafai@fb.com> This really does not make sense to me. If np was NULL, we should have a crash before. So we should remove this test, since it is absolutely useless.
On Tue, Sep 06, 2016 at 10:52:43AM -0700, Eric Dumazet wrote: > > > @@ -126,8 +126,10 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > > > rt = (struct rt6_info *) dst; > > > > > > np = inet6_sk(sk); > > > - if (!np) > > > - return -EBADF; > > > + if (!np) { > > > + err = -EBADF; > > > + goto dst_err_out; > > > + } > > > > > > if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr)) > > > fl6.flowi6_oif = np->mcast_oif; > > > @@ -163,6 +165,9 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > > > } > > > release_sock(sk); > > > > > > +dst_err_out: > > > + dst_release(dst); > > > + > > > if (err) > > > return err; > > > > > > > Acked-by: Martin KaFai Lau <kafai@fb.com> > > This really does not make sense to me. > > If np was NULL, we should have a crash before. In the case where I was seeing the traces, we were taking the 'success' path through the function, so sk was non-null. > So we should remove this test, since it is absolutely useless. Looking closer, it seems the assignment of np is duplicated also, so that can also go. This is orthogonal to the dst leak though. I'll submit a follow-up cleaning that up. Dave
From: Dave Jones <davej@codemonkey.org.uk> Date: Fri, 2 Sep 2016 14:39:50 -0400 > Neither the failure or success paths of ping_v6_sendmsg release > the dst it acquires. This leads to a flood of warnings from > "net/core/dst.c:288 dst_release" on older kernels that > don't have 8bf4ada2e21378816b28205427ee6b0e1ca4c5f1 backported. > > That patch optimistically hoped this had been fixed post 3.10, but > it seems at least one case wasn't, where I've seen this triggered > a lot from machines doing unprivileged icmp sockets. > > Cc: Martin Lau <kafai@fb.com> > Signed-off-by: Dave Jones <davej@codemonkey.org.uk> Applied and queued up for -stable, thanks Dave.
diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c index 0900352c924c..0e983b694ee8 100644 --- a/net/ipv6/ping.c +++ b/net/ipv6/ping.c @@ -126,8 +126,10 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) rt = (struct rt6_info *) dst; np = inet6_sk(sk); - if (!np) - return -EBADF; + if (!np) { + err = -EBADF; + goto dst_err_out; + } if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr)) fl6.flowi6_oif = np->mcast_oif; @@ -163,6 +165,9 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) } release_sock(sk); +dst_err_out: + dst_release(dst); + if (err) return err;
Neither the failure or success paths of ping_v6_sendmsg release the dst it acquires. This leads to a flood of warnings from "net/core/dst.c:288 dst_release" on older kernels that don't have 8bf4ada2e21378816b28205427ee6b0e1ca4c5f1 backported. That patch optimistically hoped this had been fixed post 3.10, but it seems at least one case wasn't, where I've seen this triggered a lot from machines doing unprivileged icmp sockets. Cc: Martin Lau <kafai@fb.com> Signed-off-by: Dave Jones <davej@codemonkey.org.uk>