linux-kernel - Re: Re: [PATCH V4 3/5] cachefiles: resend an open request if the read request's object is closed

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: <029453d7-1b2d-3d3c-cd8e-64f5be4420bf@bytedance.com>
Date:   Wed, 29 Mar 2023 19:42:47 +0800
From:   Jia Zhu <zhujia.zj@...edance.com>
To:     David Howells <dhowells@...hat.com>
Cc:     linux-cachefs@...hat.com, linux-erofs@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, Xin Yin <yinxin.x@...edance.com>,
        Jingbo Xu <jefflexu@...ux.alibaba.com>, zhujia.zj@...edance.com
Subject: Re: Re: [PATCH V4 3/5] cachefiles: resend an open request if the read
 request's object is closed

在 2023/3/28 22:12, David Howells 写道:
> Jia Zhu <zhujia.zj@...edance.com> wrote:
> 
>> +	struct cachefiles_object *object =
>> +		((struct cachefiles_ondemand_info *)work)->object;
> 
> container_of().
Thanks, will revise it.
> 
>> +			continue;
>> +		} else if (cachefiles_ondemand_object_is_reopening(object)) {
> 
> The "else" is unnecessary.
Will remove it.
> 
>> +static void ondemand_object_worker(struct work_struct *work)
>> +{
>> +	struct cachefiles_object *object =
>> +		((struct cachefiles_ondemand_info *)work)->object;
>> +
>> +	cachefiles_ondemand_init_object(object);
>> +}
> 
> I can't help but feel there's some missing exclusion/locking.  

It's indeed kind of complicated here since the async operation.
Thus we paid much attention to catching the race scenarios during coding
and reviewing.

Here are several corner case have been considered:

1. Don't repeatedly push the @work of same object into workqueue:
Use <reopening> state to represent this object. Once the object is
set to <reopening> atomicly, which means the work has been pushed to
workqueue. And other concurrent threads will not pick the <reopening>
object to workqueue.

2. Don't repeatedly set <reopening> state for the same object:
Hold the xa_lock during searching reqs and setting it to <reopening>.
Once object is set to <reopening>, the same object will be skipped.

3. etc.

Would you mind providing more hints for this issue?

> This feels like
> it really ought to be driven from the fscache object state machine.

It's a great idea. But the problem is if we add a new state to indicate
this reopening status and use fscache state machine to drive the cookie
to do reopen(), thus reopen() (in fscache module) ought to invoke
cachefiles_ondemand_init_object() (in cachefile module) to require user
daemon to open the backend file.
But it seems that fscache module should not depend on cachefiles module.
>