Re: ipv6: release dst in ping_v6_sendmsg
From: Dave Jones 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 > Signed-off-by: Dave Jones Applied and queued up for -stable, thanks Dave.
Re: ipv6: release dst in ping_v6_sendmsg
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
>
> 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
Re: ipv6: release dst in ping_v6_sendmsg
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
> > Signed-off-by: Dave Jones
> >
> > 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
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.
Re: ipv6: release dst in ping_v6_sendmsg
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
> Signed-off-by: Dave Jones
>
> 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
ipv6: release dst in ping_v6_sendmsg
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
Signed-off-by: Dave Jones
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;
