From: Tomas Bortoli <hidden> Date: 2018-07-23 12:19:33
A double list_del(&req->req_list) is possible in p9_fd_cancel() as
shown by Syzbot. To prevent it we have to ensure that we have the
client->lock when deleting the list. Furthermore, we have to update
the status of the request before releasing the lock, to prevent the
race.
Signed-off-by: Tomas Bortoli <redacted>
Reported-by: syzbot+735d926e9d1317c3310c@syzkaller.appspotmail.com
---
net/9p/trans_fd.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
A double list_del(&req->req_list) is possible in p9_fd_cancel() as
shown by Syzbot. To prevent it we have to ensure that we have the
client->lock when deleting the list. Furthermore, we have to update
the status of the request before releasing the lock, to prevent the
race.
Nice, so no need to change the list_del to list_del_init!
I still have a nitpick on the last moved unlock, but it's mostly
aesthetic - the change looks much better to me now.
(Since that will require a v2 I'll be evil and go further than Yiwen
about the commit message: let it breathe a bit! :) I think a line break
before "furthermore" for example will make it easier to read)
It took me a while to understand why you extended this lock despite
having just read the commit message, I'd suggest:
- moving the spin_unlock to right after p9_client_cb (afterall that's
what we want, the m->rc and m->req don't need to be protected)
- add a comment before p9_client_cb saying something like 'updates
req->status' or try to explain why it needs to be locked here but other
transports don't need such a lock (they're not dependant on req->status
like this)
--
Dominique
From: Tomas Bortoli <hidden> Date: 2018-07-23 16:51:42
On 07/23/2018 02:57 PM, Dominique Martinet wrote:
Tomas Bortoli wrote on Mon, Jul 23, 2018:
quoted
A double list_del(&req->req_list) is possible in p9_fd_cancel() as
shown by Syzbot. To prevent it we have to ensure that we have the
client->lock when deleting the list. Furthermore, we have to update
the status of the request before releasing the lock, to prevent the
race.
Nice, so no need to change the list_del to list_del_init!
I still have a nitpick on the last moved unlock, but it's mostly
aesthetic - the change looks much better to me now.
(Since that will require a v2 I'll be evil and go further than Yiwen
about the commit message: let it breathe a bit! :) I think a line break
before "furthermore" for example will make it easier to read)
It took me a while to understand why you extended this lock despite
having just read the commit message, I'd suggest:
- moving the spin_unlock to right after p9_client_cb (afterall that's
what we want, the m->rc and m->req don't need to be protected)
yes, better.
- add a comment before p9_client_cb saying something like 'updates
req->status' or try to explain why it needs to be locked here but other
transports don't need such a lock (they're not dependant on req->status
like this)
A double list_del(&req->req_list) is possible in p9_fd_cancel() as
shown by Syzbot. To prevent it we have to ensure that we have the
client->lock when deleting the list. Furthermore, we have to update
the status of the request before releasing the lock, to prevent the
race.
Signed-off-by: Tomas Bortoli <redacted>
Reported-by: syzbot+735d926e9d1317c3310c@syzkaller.appspotmail.com
---
net/9p/trans_fd.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Tomas Bortoli <hidden> Date: 2018-07-24 10:04:20
On 07/24/2018 03:40 AM, jiangyiwen wrote:
On 2018/7/23 20:19, Tomas Bortoli wrote:
quoted
A double list_del(&req->req_list) is possible in p9_fd_cancel() as
shown by Syzbot. To prevent it we have to ensure that we have the
client->lock when deleting the list. Furthermore, we have to update
the status of the request before releasing the lock, to prevent the
race.
Signed-off-by: Tomas Bortoli <redacted>
Reported-by: syzbot+735d926e9d1317c3310c@syzkaller.appspotmail.com
---
net/9p/trans_fd.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
If you want to expand the ranges of client->lock, the cancel_list will not
be necessary, you can optimize this code.
Unfortunately, not. Moving the spin_lock() before the for makes the
crash appear again. This because the calls to list_move() in the for
before delete all the elements from req->req_list, so the list is empty,
another call to list_del() would trigger a double del.
That's why we hold the lock to update the status of all those requests..
otherwise we have again the race with p9_fd_cancel().
Crash log at the bottom.
If you want to expand the ranges of client->lock, the cancel_list will not
be necessary, you can optimize this code.
Unfortunately, not. Moving the spin_lock() before the for makes the
crash appear again. This because the calls to list_move() in the for
before delete all the elements from req->req_list, so the list is empty,
another call to list_del() would trigger a double del.
That's why we hold the lock to update the status of all those requests..
otherwise we have again the race with p9_fd_cancel().
What (I think) he meant is that since you're holding the lock all the
way, you don't need to transfer all the items to a temporary list to
loop on it immediately afterwards, but you could call the client cb
directly.
I'm personally not a fan of this approach as that would duplicate the
code, even if the loop isn't big...
This code is only called at disconnect time so I think using the extra
list doesn't hurt anyone; but as usual do what you feel is better; I
don't mind much either way.
--
Dominique Martinet
If you want to expand the ranges of client->lock, the cancel_list will not
be necessary, you can optimize this code.
Unfortunately, not. Moving the spin_lock() before the for makes the
crash appear again. This because the calls to list_move() in the for
before delete all the elements from req->req_list, so the list is empty,
another call to list_del() would trigger a double del.
That's why we hold the lock to update the status of all those requests..
otherwise we have again the race with p9_fd_cancel().
What (I think) he meant is that since you're holding the lock all the
way, you don't need to transfer all the items to a temporary list to
loop on it immediately afterwards, but you could call the client cb
directly.
Yeah that is possible.
I'm personally not a fan of this approach as that would duplicate the
code, even if the loop isn't big...
Yep
This code is only called at disconnect time so I think using the extra
list doesn't hurt anyone; but as usual do what you feel is better; I
don't mind much either way.