From: 周多明 <duoming@zju.edu.cn>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: linux-hams@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, jreuter@yaina.de, kuba@kernel.org,
davem@davemloft.net, ralf@linux-mips.org, thomas@osterried.de
Subject: Re: Re: Re: [PATCH V3] ax25: Fix refcount leaks caused by ax25_cb_del()
Date: Tue, 15 Mar 2022 09:14:46 +0800 (GMT+08:00) [thread overview]
Message-ID: <5bf6c167.1ed9.17f8b244e74.Coremail.duoming@zju.edu.cn> (raw)
In-Reply-To: <20220314110256.GL3293@kadam>
Hello,
On Mon, 14 Mar 2022 14:02:56 +0300, Dan Carpen wrote:
> > > But even here, my instinct is that if the refcounting is were done in
> > > the correct place we would not need any additional variables. Is there
> > > no simpler solution?
I think there is a simpler solution instead of using any additional variables,
which is shown below:
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 6bd09718077..0886109421a 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -98,8 +98,10 @@ static void ax25_kill_by_device(struct net_device *dev)
spin_unlock_bh(&ax25_list_lock);
lock_sock(sk);
s->ax25_dev = NULL;
- dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
- ax25_dev_put(ax25_dev);
+ if (sk->sk_wq) {
+ dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+ ax25_dev_put(ax25_dev);
+ }
ax25_disconnect(s, ENETUNREACH);
release_sock(sk);
spin_lock_bh(&ax25_list_lock);
@@ -979,14 +981,20 @@ static int ax25_release(struct socket *sock)
{
struct sock *sk = sock->sk;
ax25_cb *ax25;
+ ax25_dev *ax25_dev;
if (sk == NULL)
return 0;
sock_hold(sk);
- sock_orphan(sk);
lock_sock(sk);
+ sock_orphan(sk);
ax25 = sk_to_ax25(sk);
+ ax25_dev = ax25->ax25_dev;
+ if (ax25_dev) {
+ dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+ ax25_dev_put(ax25_dev);
+ }
if (sk->sk_type == SOCK_SEQPACKET) {
switch (ax25->state) {
we add decrements of refcounts in ax25_release(), and use lock_sock() to do synchronization.
If refcounts decrease in ax25_release(), the decrements of refcounts in ax25_kill_by_device()
will not be executed and vice versa.
1. If we decrease the refcounts in ax25_release(), the decrements of refcounts in
ax25_kill_by_device() will not execute. Because we set NULL to sk->sk_wq in sock_orphan()
and we check whether sk->sk_wq is NULL in ax25_kill_by_device(). Only positions (3) and (4)
could execute.
(Thread 1) |
ax25_bind() |
... |
ax25_addr_ax25dev() |
ax25_dev_hold() //(1) |
... |
dev_hold_track() //(2) | (Thread 2)
... | ax25_release()
| ...
| lock_sock(sk)
| sock_orphan(sk)
| sk->sk_wq = NULL //set NULL
| ...
| if (ax25_dev) {
| dev_put_track() //(3)
| ax25_dev_put() //(4)
|
(thread 3) |
ax25_kill_by_device() |
... |
lock_sock(sk) |
s->ax25_dev = NULL |
if (sk->sk_wq) { //check |
dev_put_track() //(5) |
ax25_dev_put() //(6) |
2. If we decrease refcounts in ax25_kill_by_device(), the decrements of refcounts
in ax25_release() will not execute. Because we set NULL to s->ax25_dev in ax25_kill_by_device()
and we check whether ax25_dev is NULL in ax25_release(). Only positions (3) and (4)
could execute.
(Thread 1) |
ax25_bind() |
... |
ax25_addr_ax25dev() |
ax25_dev_hold() //(1) |
... |
dev_hold_track() //(2) | (Thread 2)
... | ax25_kill_by_device()
| ...
| lock_sock(sk)
| s->ax25_dev = NULL //set NULL
| if (sk->sk_wq) {
| dev_put_track() //(3)
| ax25_dev_put() //(4)
| ...
|
(thread 3) |
ax25_release() |
... |
lock_sock(sk); |
sock_orphan(sk); |
... |
if (ax25_dev) { //check |
dev_put_track() //(5) |
ax25_dev_put() //(6) |
> > I sent "[PATCH net V2 1/2] ax25: Fix refcount leaks caused by ax25_cb_del()"
> > on On Fri, Mar 11, 2022. Could this patch solve your question?
>
> I had a bunch of questions... You just ignored them, and sent a patch
> called v2 instead of v4 so I was puzzled and confused. I guess the
> answer is no, could you please answer the questions?
I hope my answer could solve your questions. If you still have any questions
welcome to send email to me. I will send "[PATCH net V4 1/2] ax25: Fix
refcount leaks caused by ax25_cb_del()" as soon as possible.
What`s more, I found NPD bugs in ax25 timers, I will send
"[PATCH net V4 2/2] ax25: Fix NULL pointer dereferences in ax25 timers" together.
Best wishes,
Duoming Zhou
prev parent reply other threads:[~2022-03-15 1:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-11 1:46 [PATCH V3] ax25: Fix refcount leaks caused by ax25_cb_del() Duoming Zhou
2022-03-11 10:53 ` Dan Carpenter
2022-03-13 4:26 ` 周多明
2022-03-14 11:02 ` Dan Carpenter
2022-03-15 1:14 ` 周多明 [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5bf6c167.1ed9.17f8b244e74.Coremail.duoming@zju.edu.cn \
--to=duoming@zju.edu.cn \
--cc=dan.carpenter@oracle.com \
--cc=davem@davemloft.net \
--cc=jreuter@yaina.de \
--cc=kuba@kernel.org \
--cc=linux-hams@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=ralf@linux-mips.org \
--cc=thomas@osterried.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).