linux-kernel - Re: [PATCH V2] block: rename 'q->debugfs_dir' and 'q->blk_trace->dir' in blk_unregister

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: <96287511-a121-02f5-c5e0-24873fd30179@huawei.com>
Date:   Sat, 29 Feb 2020 10:50:56 +0800
From:   "yukuai (C)" <yukuai3@...wei.com>
To:     Ming Lei <ming.lei@...hat.com>
CC:     <axboe@...nel.dk>, <chaitanya.kulkarni@....com>,
        <damien.lemoal@....com>, <bvanassche@....org>,
        <dhowells@...hat.com>, <asml.silence@...il.com>,
        <ajay.joshi@....com>, <linux-block@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <yi.zhang@...wei.com>,
        <zhangxiaoxu5@...wei.com>, <luoshijie1@...wei.com>
Subject: Re: [PATCH V2] block: rename 'q->debugfs_dir' and 'q->blk_trace->dir'
 in blk_unregister_queue()

On 2020/2/29 7:15, Ming Lei wrote:
> If blk_trace->dir isn't same with .debugfs_dir, you will just rename
> blk_trace->dir, this way can't avoid the failure in step3, can it?
Thank you for your response!
It's true that the problem still exist if they are not the same（I
thougt they can't both exist）. Perhaps I can do the renaming for both.
> 
> I understand that we just need to rename .debugfs_dir, meantime making
> sure blktrace code removes correct debugfs dir, is that enough for fixing
> this issue?
> 
>> +	if (old == NULL)
>> +		return;
>> +	while (new == NULL) {
>> +		sprintf(name, "ready_to_remove_%s_%d",
>> +				kobject_name(q->kobj.parent), i++);
>> +		new = debugfs_rename(blk_debugfs_root, *old,
>> +				     blk_debugfs_root, name);
>> +	}
> The above code can be run concurrently with blktrace shutdown, so race might
> exit between the two code paths, then bt->dir may has been renamed or being
> renamed in debugfs_remove(bt->dir), can this function work as expected or
> correct?
To avoid the race, I think we can take the lock 'blk_trace_mutex' while
renaming 'bt->dir'.
> 
> And there is dead loop risk, so suggest to not rename this way.
The deap loop will exist if 'debugfs_rename' keep failing for some
reason. I thougt about setting a limit for max loop count, however, It's
probably not a good solution.

Thanks!
Yu Kuai
>