linux-kernel - Re: [PATCH] net/p9/trans_fd.c: fix double list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2e02c44a-0775-dc74-c3a9-3c41759d182b@gmail.com>
Date:   Tue, 24 Jul 2018 12:47:44 +0200
From:   Tomas Bortoli <tomasbortoli@...il.com>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     jiangyiwen <jiangyiwen@...wei.com>, davem@...emloft.net,
        v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller@...glegroups.com
Subject: Re: [PATCH] net/p9/trans_fd.c: fix double list_del()

On 07/24/2018 12:19 PM, Dominique Martinet wrote:
> Tomas Bortoli wrote on Tue, Jul 24, 2018:
>>>> @@ -228,6 +226,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
>>>>  			req->t_err = err;
>>>>  		p9_client_cb(m->client, req, REQ_STATUS_ERROR);
>>>>  	}
>>>> +	spin_unlock(&m->client->lock);
>>>
>>> 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.
> 

I think it's fine as it is.