* [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()
@ 2022-03-23 0:41 Eric Dumazet
2022-03-23 18:00 ` patchwork-bot+netdevbpf
2022-03-24 6:22 ` Dan Carpenter
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-03-23 0:41 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, Eric Dumazet, Eric Dumazet, 赵子轩,
Stoyan Manolov
From: Eric Dumazet <edumazet@google.com>
Whenever llc_ui_bind() and/or llc_ui_autobind()
took a reference on a netdevice but subsequently fail,
they must properly release their reference
or risk the infamous message from unregister_netdevice()
at device dismantle.
unregister_netdevice: waiting for eth0 to become free. Usage count = 3
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: 赵子轩 <beraphin@gmail.com>
Reported-by: Stoyan Manolov <smanolov@suse.de>
---
This can be applied on net tree, depending on how network maintainers
plan to push the fix to Linus, this is obviously a stable candidate.
net/llc/af_llc.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index 26c00ebf4fbae4d7dc1c27d180385470fa252be0..c86256064743523f0621f21d5d023956cf1df9a0 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -311,6 +311,10 @@ static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
sock_reset_flag(sk, SOCK_ZAPPED);
rc = 0;
out:
+ if (rc) {
+ dev_put_track(llc->dev, &llc->dev_tracker);
+ llc->dev = NULL;
+ }
return rc;
}
@@ -408,6 +412,10 @@ static int llc_ui_bind(struct socket *sock, struct sockaddr *uaddr, int addrlen)
out_put:
llc_sap_put(sap);
out:
+ if (rc) {
+ dev_put_track(llc->dev, &llc->dev_tracker);
+ llc->dev = NULL;
+ }
release_sock(sk);
return rc;
}
--
2.35.1.894.gb6a874cedc-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()
2022-03-23 0:41 [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind() Eric Dumazet
@ 2022-03-23 18:00 ` patchwork-bot+netdevbpf
2022-03-24 6:22 ` Dan Carpenter
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-03-23 18:00 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, pabeni, netdev, edumazet, beraphin, smanolov
Hello:
This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 22 Mar 2022 17:41:47 -0700 you wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Whenever llc_ui_bind() and/or llc_ui_autobind()
> took a reference on a netdevice but subsequently fail,
> they must properly release their reference
> or risk the infamous message from unregister_netdevice()
> at device dismantle.
>
> [...]
Here is the summary with links:
- [net-next] llc: fix netdevice reference leaks in llc_ui_bind()
https://git.kernel.org/netdev/net-next/c/764f4eb6846f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()
2022-03-23 0:41 [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind() Eric Dumazet
2022-03-23 18:00 ` patchwork-bot+netdevbpf
@ 2022-03-24 6:22 ` Dan Carpenter
2022-03-24 14:38 ` Eric Dumazet
1 sibling, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-03-24 6:22 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
Eric Dumazet, 赵子轩,
Stoyan Manolov
On Tue, Mar 22, 2022 at 05:41:47PM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Whenever llc_ui_bind() and/or llc_ui_autobind()
> took a reference on a netdevice but subsequently fail,
> they must properly release their reference
> or risk the infamous message from unregister_netdevice()
> at device dismantle.
>
> unregister_netdevice: waiting for eth0 to become free. Usage count = 3
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: 赵子轩 <beraphin@gmail.com>
> Reported-by: Stoyan Manolov <smanolov@suse.de>
> ---
>
> This can be applied on net tree, depending on how network maintainers
> plan to push the fix to Linus, this is obviously a stable candidate.
This patch is fine, but it's that function is kind of ugly and difficult
for static analysis to parse.
net/llc/af_llc.c
274 static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
275 {
276 struct sock *sk = sock->sk;
277 struct llc_sock *llc = llc_sk(sk);
278 struct llc_sap *sap;
279 int rc = -EINVAL;
280
281 if (!sock_flag(sk, SOCK_ZAPPED))
282 goto out;
This condition is checking to see if someone else already initialized
llc->dev. If we call dev_put_track(llc->dev, &llc->dev_tracker) on
something we didn't allocate then it leads to a use after free. But
fortunately the callers all check SOCK_ZAPPED so the condition is
impossible.
283 if (!addr->sllc_arphrd)
284 addr->sllc_arphrd = ARPHRD_ETHER;
285 if (addr->sllc_arphrd != ARPHRD_ETHER)
286 goto out;
Thus we know that "llc->dev" is NULL on these first couple gotos and
calling dev_put_track(llc->dev, &llc->dev_tracker); is a no-op so it's
fine.
But complicated to review.
287 rc = -ENODEV;
288 if (sk->sk_bound_dev_if) {
289 llc->dev = dev_get_by_index(&init_net, sk->sk_bound_dev_if);
290 if (llc->dev && addr->sllc_arphrd != llc->dev->type) {
291 dev_put(llc->dev);
292 llc->dev = NULL;
293 }
294 } else
295 llc->dev = dev_getfirstbyhwtype(&init_net, addr->sllc_arphrd);
296 if (!llc->dev)
297 goto out;
298 netdev_tracker_alloc(llc->dev, &llc->dev_tracker, GFP_KERNEL);
299 rc = -EUSERS;
300 llc->laddr.lsap = llc_ui_autoport();
301 if (!llc->laddr.lsap)
302 goto out;
303 rc = -EBUSY; /* some other network layer is using the sap */
304 sap = llc_sap_open(llc->laddr.lsap, NULL);
305 if (!sap)
306 goto out;
307 memcpy(llc->laddr.mac, llc->dev->dev_addr, IFHWADDRLEN);
308 memcpy(&llc->addr, addr, sizeof(llc->addr));
309 /* assign new connection to its SAP */
310 llc_sap_add_socket(sap, sk);
311 sock_reset_flag(sk, SOCK_ZAPPED);
312 rc = 0;
313 out:
314 if (rc) {
315 dev_put_track(llc->dev, &llc->dev_tracker);
316 llc->dev = NULL;
317 }
318 return rc;
319 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()
2022-03-24 6:22 ` Dan Carpenter
@ 2022-03-24 14:38 ` Eric Dumazet
2022-03-24 16:25 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2022-03-24 14:38 UTC (permalink / raw)
To: Dan Carpenter
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni,
netdev, 赵子轩,
Stoyan Manolov
On Wed, Mar 23, 2022 at 11:23 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Mar 22, 2022 at 05:41:47PM -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Whenever llc_ui_bind() and/or llc_ui_autobind()
> > took a reference on a netdevice but subsequently fail,
> > they must properly release their reference
> > or risk the infamous message from unregister_netdevice()
> > at device dismantle.
> >
> > unregister_netdevice: waiting for eth0 to become free. Usage count = 3
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Reported-by: 赵子轩 <beraphin@gmail.com>
> > Reported-by: Stoyan Manolov <smanolov@suse.de>
> > ---
> >
> > This can be applied on net tree, depending on how network maintainers
> > plan to push the fix to Linus, this is obviously a stable candidate.
>
> This patch is fine, but it's that function is kind of ugly and difficult
> for static analysis to parse.
We usually do not mix bug fixes and code refactoring.
Please feel free to send a refactor when net-next reopens in two weeks.
Thanks.
>
> net/llc/af_llc.c
> 274 static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
> 275 {
> 276 struct sock *sk = sock->sk;
> 277 struct llc_sock *llc = llc_sk(sk);
> 278 struct llc_sap *sap;
> 279 int rc = -EINVAL;
> 280
> 281 if (!sock_flag(sk, SOCK_ZAPPED))
> 282 goto out;
>
> This condition is checking to see if someone else already initialized
> llc->dev. If we call dev_put_track(llc->dev, &llc->dev_tracker) on
> something we didn't allocate then it leads to a use after free. But
> fortunately the callers all check SOCK_ZAPPED so the condition is
> impossible.
>
> 283 if (!addr->sllc_arphrd)
> 284 addr->sllc_arphrd = ARPHRD_ETHER;
> 285 if (addr->sllc_arphrd != ARPHRD_ETHER)
> 286 goto out;
>
> Thus we know that "llc->dev" is NULL on these first couple gotos and
> calling dev_put_track(llc->dev, &llc->dev_tracker); is a no-op so it's
> fine.
>
> But complicated to review.
>
> 287 rc = -ENODEV;
> 288 if (sk->sk_bound_dev_if) {
> 289 llc->dev = dev_get_by_index(&init_net, sk->sk_bound_dev_if);
> 290 if (llc->dev && addr->sllc_arphrd != llc->dev->type) {
> 291 dev_put(llc->dev);
> 292 llc->dev = NULL;
> 293 }
> 294 } else
> 295 llc->dev = dev_getfirstbyhwtype(&init_net, addr->sllc_arphrd);
> 296 if (!llc->dev)
> 297 goto out;
> 298 netdev_tracker_alloc(llc->dev, &llc->dev_tracker, GFP_KERNEL);
> 299 rc = -EUSERS;
> 300 llc->laddr.lsap = llc_ui_autoport();
> 301 if (!llc->laddr.lsap)
> 302 goto out;
> 303 rc = -EBUSY; /* some other network layer is using the sap */
> 304 sap = llc_sap_open(llc->laddr.lsap, NULL);
> 305 if (!sap)
> 306 goto out;
> 307 memcpy(llc->laddr.mac, llc->dev->dev_addr, IFHWADDRLEN);
> 308 memcpy(&llc->addr, addr, sizeof(llc->addr));
> 309 /* assign new connection to its SAP */
> 310 llc_sap_add_socket(sap, sk);
> 311 sock_reset_flag(sk, SOCK_ZAPPED);
> 312 rc = 0;
> 313 out:
> 314 if (rc) {
> 315 dev_put_track(llc->dev, &llc->dev_tracker);
> 316 llc->dev = NULL;
> 317 }
> 318 return rc;
> 319 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()
2022-03-24 14:38 ` Eric Dumazet
@ 2022-03-24 16:25 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-03-24 16:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni,
netdev, 赵子轩,
Stoyan Manolov
On Thu, Mar 24, 2022 at 7:38 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Mar 23, 2022 at 11:23 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Mar 22, 2022 at 05:41:47PM -0700, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > Whenever llc_ui_bind() and/or llc_ui_autobind()
> > > took a reference on a netdevice but subsequently fail,
> > > they must properly release their reference
> > > or risk the infamous message from unregister_netdevice()
> > > at device dismantle.
> > >
> > > unregister_netdevice: waiting for eth0 to become free. Usage count = 3
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Reported-by: 赵子轩 <beraphin@gmail.com>
> > > Reported-by: Stoyan Manolov <smanolov@suse.de>
> > > ---
> > >
> > > This can be applied on net tree, depending on how network maintainers
> > > plan to push the fix to Linus, this is obviously a stable candidate.
> >
> > This patch is fine, but it's that function is kind of ugly and difficult
> > for static analysis to parse.
>
> We usually do not mix bug fixes and code refactoring.
>
> Please feel free to send a refactor when net-next reopens in two weeks.
>
> Thanks.
I took another look at this code, and there might be an issue in llc_ui_bind(),
if the "goto out;" is taken _before_ we took a reference on a device.
We might now release the reference taken by a prior (and successful bind)
So it turns out another fix is needed.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-24 16:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-23 0:41 [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind() Eric Dumazet
2022-03-23 18:00 ` patchwork-bot+netdevbpf
2022-03-24 6:22 ` Dan Carpenter
2022-03-24 14:38 ` Eric Dumazet
2022-03-24 16:25 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).