| Message ID | 102f4ec1ec622e054bb226f7b31e739c31e795ff.1552029955.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] pptp: dst_release sk_dst_cache in pptp_sock_destruct | expand |
On 03/07/2019 11:25 PM, Xin Long wrote: > sk_setup_caps() is called to set sk->sk_dst_cache in pptp_connect, > so we have to dst_release(sk->sk_dst_cache) in pptp_sock_destruct, > otherwise, the dst refcnt will leak. > > It can be reproduced by this syz log: > > r1 = socket$pptp(0x18, 0x1, 0x2) > bind$pptp(r1, &(0x7f0000000100)={0x18, 0x2, {0x0, @local}}, 0x1e) > connect$pptp(r1, &(0x7f0000000000)={0x18, 0x2, {0x3, @remote}}, 0x1e) > > Consecutive dmesg warnings will occur: > > unregister_netdevice: waiting for lo to become free. Usage count = 1 > > Fixes: 00959ade36ac ("PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)") > Reported-by: Xiumei Mu <xmu@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > drivers/net/ppp/pptp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c > index 8f09edd..76172c2 100644 > --- a/drivers/net/ppp/pptp.c > +++ b/drivers/net/ppp/pptp.c > @@ -532,6 +532,7 @@ static void pptp_sock_destruct(struct sock *sk) > pppox_unbind_sock(sk); > } > skb_queue_purge(&sk->sk_receive_queue); > + dst_release(rcu_dereference_check(sk->sk_dst_cache, 1)); rcu_dereference_protected() is probably more appropriate. (No barrier is needed) > } > > static int pptp_create(struct net *net, struct socket *sock, int kern) >
On 03/08/2019 09:17 AM, Eric Dumazet wrote: > > > On 03/07/2019 11:25 PM, Xin Long wrote: >> sk_setup_caps() is called to set sk->sk_dst_cache in pptp_connect, >> so we have to dst_release(sk->sk_dst_cache) in pptp_sock_destruct, >> otherwise, the dst refcnt will leak. >> >> It can be reproduced by this syz log: >> >> r1 = socket$pptp(0x18, 0x1, 0x2) >> bind$pptp(r1, &(0x7f0000000100)={0x18, 0x2, {0x0, @local}}, 0x1e) >> connect$pptp(r1, &(0x7f0000000000)={0x18, 0x2, {0x3, @remote}}, 0x1e) >> >> Consecutive dmesg warnings will occur: >> >> unregister_netdevice: waiting for lo to become free. Usage count = 1 >> >> Fixes: 00959ade36ac ("PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)") >> Reported-by: Xiumei Mu <xmu@redhat.com> >> Signed-off-by: Xin Long <lucien.xin@gmail.com> >> --- >> drivers/net/ppp/pptp.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c >> index 8f09edd..76172c2 100644 >> --- a/drivers/net/ppp/pptp.c >> +++ b/drivers/net/ppp/pptp.c >> @@ -532,6 +532,7 @@ static void pptp_sock_destruct(struct sock *sk) >> pppox_unbind_sock(sk); >> } >> skb_queue_purge(&sk->sk_receive_queue); >> + dst_release(rcu_dereference_check(sk->sk_dst_cache, 1)); > > rcu_dereference_protected() is probably more appropriate. (No barrier is needed) Before you ask, the first instance of this line probably came in commit b6c6712a42ca3f9fa7f4a3d7c40e3a9dd1fd9e03 (net: sk_dst_cache RCUification) but at that time rcu_dereference_protected() was not there yet. We might cleanup this for all callers.
On Sat, Mar 9, 2019 at 1:17 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 03/07/2019 11:25 PM, Xin Long wrote: > > sk_setup_caps() is called to set sk->sk_dst_cache in pptp_connect, > > so we have to dst_release(sk->sk_dst_cache) in pptp_sock_destruct, > > otherwise, the dst refcnt will leak. > > > > It can be reproduced by this syz log: > > > > r1 = socket$pptp(0x18, 0x1, 0x2) > > bind$pptp(r1, &(0x7f0000000100)={0x18, 0x2, {0x0, @local}}, 0x1e) > > connect$pptp(r1, &(0x7f0000000000)={0x18, 0x2, {0x3, @remote}}, 0x1e) > > > > Consecutive dmesg warnings will occur: > > > > unregister_netdevice: waiting for lo to become free. Usage count = 1 > > > > Fixes: 00959ade36ac ("PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)") > > Reported-by: Xiumei Mu <xmu@redhat.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > drivers/net/ppp/pptp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c > > index 8f09edd..76172c2 100644 > > --- a/drivers/net/ppp/pptp.c > > +++ b/drivers/net/ppp/pptp.c > > @@ -532,6 +532,7 @@ static void pptp_sock_destruct(struct sock *sk) > > pppox_unbind_sock(sk); > > } > > skb_queue_purge(&sk->sk_receive_queue); > > + dst_release(rcu_dereference_check(sk->sk_dst_cache, 1)); > > rcu_dereference_protected() is probably more appropriate. (No barrier is needed) Seems so, the update is prevented then. I will post v2. Thanks. > > > } > > > > static int pptp_create(struct net *net, struct socket *sock, int kern) > >
On Sat, Mar 9, 2019 at 1:22 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 03/08/2019 09:17 AM, Eric Dumazet wrote: > > > > > > On 03/07/2019 11:25 PM, Xin Long wrote: > >> sk_setup_caps() is called to set sk->sk_dst_cache in pptp_connect, > >> so we have to dst_release(sk->sk_dst_cache) in pptp_sock_destruct, > >> otherwise, the dst refcnt will leak. > >> > >> It can be reproduced by this syz log: > >> > >> r1 = socket$pptp(0x18, 0x1, 0x2) > >> bind$pptp(r1, &(0x7f0000000100)={0x18, 0x2, {0x0, @local}}, 0x1e) > >> connect$pptp(r1, &(0x7f0000000000)={0x18, 0x2, {0x3, @remote}}, 0x1e) > >> > >> Consecutive dmesg warnings will occur: > >> > >> unregister_netdevice: waiting for lo to become free. Usage count = 1 > >> > >> Fixes: 00959ade36ac ("PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)") > >> Reported-by: Xiumei Mu <xmu@redhat.com> > >> Signed-off-by: Xin Long <lucien.xin@gmail.com> > >> --- > >> drivers/net/ppp/pptp.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c > >> index 8f09edd..76172c2 100644 > >> --- a/drivers/net/ppp/pptp.c > >> +++ b/drivers/net/ppp/pptp.c > >> @@ -532,6 +532,7 @@ static void pptp_sock_destruct(struct sock *sk) > >> pppox_unbind_sock(sk); > >> } > >> skb_queue_purge(&sk->sk_receive_queue); > >> + dst_release(rcu_dereference_check(sk->sk_dst_cache, 1)); > > > > rcu_dereference_protected() is probably more appropriate. (No barrier is needed) > > Before you ask, the first instance of this line probably came in commit > b6c6712a42ca3f9fa7f4a3d7c40e3a9dd1fd9e03 (net: sk_dst_cache RCUification) > but at that time rcu_dereference_protected() was not there yet. > > We might cleanup this for all callers. > I will do it in another patch for dn_destruct() and inet_sock_destruct().
diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c index 8f09edd..76172c2 100644 --- a/drivers/net/ppp/pptp.c +++ b/drivers/net/ppp/pptp.c @@ -532,6 +532,7 @@ static void pptp_sock_destruct(struct sock *sk) pppox_unbind_sock(sk); } skb_queue_purge(&sk->sk_receive_queue); + dst_release(rcu_dereference_check(sk->sk_dst_cache, 1)); } static int pptp_create(struct net *net, struct socket *sock, int kern)
sk_setup_caps() is called to set sk->sk_dst_cache in pptp_connect, so we have to dst_release(sk->sk_dst_cache) in pptp_sock_destruct, otherwise, the dst refcnt will leak. It can be reproduced by this syz log: r1 = socket$pptp(0x18, 0x1, 0x2) bind$pptp(r1, &(0x7f0000000100)={0x18, 0x2, {0x0, @local}}, 0x1e) connect$pptp(r1, &(0x7f0000000000)={0x18, 0x2, {0x3, @remote}}, 0x1e) Consecutive dmesg warnings will occur: unregister_netdevice: waiting for lo to become free. Usage count = 1 Fixes: 00959ade36ac ("PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)") Reported-by: Xiumei Mu <xmu@redhat.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- drivers/net/ppp/pptp.c | 1 + 1 file changed, 1 insertion(+)