[PATCH v2 0/5] cachefiles: some bugfixes for clean object/send req/poll

[PATCH v2 0/5] cachefiles: some bugfixes for clean object/send req/poll

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Hi all!

This is the second version of this patch series. Thank you, Jia Zhu and
Gao Xiang, for the feedback in the previous version.

We've been testing ondemand mode for cachefiles since January, and we're
almost done. We hit a lot of issues during the testing period, and this
patch set fixes some of the issues related to reopen worker/send req/poll.
The patches have passed internal testing without regression.

Patch 1-3: A read request waiting for reopen could be closed maliciously
before the reopen worker is executing or waiting to be scheduled. So
ondemand_object_worker() may be called after the info and object and even
the cache have been freed and trigger use-after-free. So use
cancel_work_sync() in cachefiles_ondemand_clean_object() to cancel the
reopen worker or wait for it to finish. Since it makes no sense to wait
for the daemon to complete the reopen request, to avoid this pointless
operation blocking cancel_work_sync(), Patch 1 avoids request generation
by the DROPPING state when the request has not been sent, and Patch 2
flushes the requests of the current object before cancel_work_sync().

Patch 4: Cyclic allocation of msg_id to avoid msg_id reuse misleading
the daemon to cause hung.

Patch 5: Hold xas_lock during polling to avoid dereferencing reqs causing
use-after-free. This issue was triggered frequently in our tests, and we
found that anolis 5.10 had fixed it, so to avoid failing the test, this
patch was pushed upstream as well.

Comments and questions are, as always, welcome.
Please let me know what you think.

Thanks,
Baokun

Changes since v1:
  * Collect RVB from Jia Zhu and Gao Xiang.(Thanks for your review!)
  * Pathch 1,2：Add more commit messages.
  * Pathch 3：Add Fixes tag as suggested by Jia Zhu.
  * Pathch 4：No longer changing "do...while" to "retry" to focus changes
    and optimise commit messages.
  * Pathch 5: Drop the internal RVB tag.

[V1]: https://lore.kernel.org/all/20240424033409.2735257-1-libaokun@huaweicloud.com

Baokun Li (3):
  cachefiles: stop sending new request when dropping object
  cachefiles: flush all requests for the object that is being dropped
  cachefiles: cyclic allocation of msg_id to avoid reuse

Hou Tao (1):
  cachefiles: flush ondemand_object_worker during clean object

Jingbo Xu (1):
  cachefiles: add missing lock protection when polling

 fs/cachefiles/daemon.c   |  4 ++--
 fs/cachefiles/internal.h |  3 +++
 fs/cachefiles/ondemand.c | 52 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 51 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH v2 1/5] cachefiles: stop sending new request when dropping object

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Added CACHEFILES_ONDEMAND_OBJSTATE_DROPPING indicates that the cachefiles
object is being dropped, and is set after the close request for the dropped
object completes, and no new requests are allowed to be sent after this
state.

This prepares for the later addition of cancel_work_sync(). It prevents
leftover reopen requests from being sent, to avoid processing unnecessary
requests and to avoid cancel_work_sync() blocking by waiting for daemon to
complete the reopen requests.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/cachefiles/internal.h |  2 ++
 fs/cachefiles/ondemand.c | 10 ++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index d33169f0018b..8ecd296cc1c4 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -48,6 +48,7 @@ enum cachefiles_object_state {
 	CACHEFILES_ONDEMAND_OBJSTATE_CLOSE, /* Anonymous fd closed by daemon or initial state */
 	CACHEFILES_ONDEMAND_OBJSTATE_OPEN, /* Anonymous fd associated with object is available */
 	CACHEFILES_ONDEMAND_OBJSTATE_REOPENING, /* Object that was closed and is being reopened. */
+	CACHEFILES_ONDEMAND_OBJSTATE_DROPPING, /* Object is being dropped. */
 };
 
 struct cachefiles_ondemand_info {
@@ -332,6 +333,7 @@ cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
 CACHEFILES_OBJECT_STATE_FUNCS(open, OPEN);
 CACHEFILES_OBJECT_STATE_FUNCS(close, CLOSE);
 CACHEFILES_OBJECT_STATE_FUNCS(reopening, REOPENING);
+CACHEFILES_OBJECT_STATE_FUNCS(dropping, DROPPING);
 
 static inline bool cachefiles_ondemand_is_reopening_read(struct cachefiles_req *req)
 {
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 4ba42f1fa3b4..73da4d4eaa9b 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -422,7 +422,8 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 		 */
 		xas_lock(&xas);
 
-		if (test_bit(CACHEFILES_DEAD, &cache->flags)) {
+		if (test_bit(CACHEFILES_DEAD, &cache->flags) ||
+		    cachefiles_ondemand_object_is_dropping(object)) {
 			xas_unlock(&xas);
 			ret = -EIO;
 			goto out;
@@ -463,7 +464,8 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 	 * If error occurs after creating the anonymous fd,
 	 * cachefiles_ondemand_fd_release() will set object to close.
 	 */
-	if (opcode == CACHEFILES_OP_OPEN)
+	if (opcode == CACHEFILES_OP_OPEN &&
+	    !cachefiles_ondemand_object_is_dropping(object))
 		cachefiles_ondemand_set_object_close(object);
 	kfree(req);
 	return ret;
@@ -562,8 +564,12 @@ int cachefiles_ondemand_init_object(struct cachefiles_object *object)
 
 void cachefiles_ondemand_clean_object(struct cachefiles_object *object)
 {
+	if (!object->ondemand)
+		return;
+
 	cachefiles_ondemand_send_req(object, CACHEFILES_OP_CLOSE, 0,
 			cachefiles_ondemand_init_close_req, NULL);
+	cachefiles_ondemand_set_object_dropping(object);
 }
 
 int cachefiles_ondemand_init_obj_info(struct cachefiles_object *object,
-- 
2.39.2

[PATCH v2 2/5] cachefiles: flush all requests for the object that is being dropped

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Because after an object is dropped, requests for that object are useless,
flush them to avoid causing other problems.

This prepares for the later addition of cancel_work_sync(). After the
reopen requests is generated, flush it to avoid cancel_work_sync()
blocking by waiting for daemon to complete the reopen requests.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/cachefiles/ondemand.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 73da4d4eaa9b..d24bff43499b 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -564,12 +564,31 @@ int cachefiles_ondemand_init_object(struct cachefiles_object *object)
 
 void cachefiles_ondemand_clean_object(struct cachefiles_object *object)
 {
+	unsigned long index;
+	struct cachefiles_req *req;
+	struct cachefiles_cache *cache;
+
 	if (!object->ondemand)
 		return;
 
 	cachefiles_ondemand_send_req(object, CACHEFILES_OP_CLOSE, 0,
 			cachefiles_ondemand_init_close_req, NULL);
+
+	if (!object->ondemand->ondemand_id)
+		return;
+
+	/* Flush all requests for the object that is being dropped. */
+	cache = object->volume->cache;
+	xa_lock(&cache->reqs);
 	cachefiles_ondemand_set_object_dropping(object);
+	xa_for_each(&cache->reqs, index, req) {
+		if (req->object == object) {
+			req->error = -EIO;
+			complete(&req->done);
+			__xa_erase(&cache->reqs, index);
+		}
+	}
+	xa_unlock(&cache->reqs);
 }
 
 int cachefiles_ondemand_init_obj_info(struct cachefiles_object *object,
-- 
2.39.2

[PATCH v2 3/5] cachefiles: flush ondemand_object_worker during clean object

[libaokun]

From: Hou Tao <houtao1@huawei.com>

When queuing ondemand_object_worker() to re-open the object,
cachefiles_object is not pinned. The cachefiles_object may be freed when
the pending read request is completed intentionally and the related
erofs is umounted. If ondemand_object_worker() runs after the object is
freed, it will incur use-after-free problem as shown below.

process A  processs B  process C  process D

cachefiles_ondemand_send_req()
// send a read req X
// wait for its completion

           // close ondemand fd
           cachefiles_ondemand_fd_release()
           // set object as CLOSE

                       cachefiles_ondemand_daemon_read()
                       // set object as REOPENING
                       queue_work(fscache_wq, &info->ondemand_work)

                                // close /dev/cachefiles
                                cachefiles_daemon_release
                                cachefiles_flush_reqs
                                complete(&req->done)

// read req X is completed
// umount the erofs fs
cachefiles_put_object()
// object will be freed
cachefiles_ondemand_deinit_obj_info()
kmem_cache_free(object)
                       // both info and object are freed
                       ondemand_object_worker()

When dropping an object, it is no longer necessary to reopen the object,
so use cancel_work_sync() to cancel or wait for ondemand_object_worker()
to complete.

Fixes: 0a7e54c1959c ("cachefiles: resend an open request if the read request's object is closed")
Signed-off-by: Hou Tao <houtao1@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com>
---
 fs/cachefiles/ondemand.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index d24bff43499b..f6440b3e7368 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -589,6 +589,9 @@ void cachefiles_ondemand_clean_object(struct cachefiles_object *object)
 		}
 	}
 	xa_unlock(&cache->reqs);
+
+	/* Wait for ondemand_object_worker() to finish to avoid UAF. */
+	cancel_work_sync(&object->ondemand->ondemand_work);
 }
 
 int cachefiles_ondemand_init_obj_info(struct cachefiles_object *object,
-- 
2.39.2

[PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[libaokun]

From: Baokun Li <libaokun1@huawei.com>

Reusing the msg_id after a maliciously completed reopen request may cause
a read request to remain unprocessed and result in a hung, as shown below:

       t1       |      t2       |      t3
-------------------------------------------------
cachefiles_ondemand_select_req
 cachefiles_ondemand_object_is_close(A)
 cachefiles_ondemand_set_object_reopening(A)
 queue_work(fscache_object_wq, &info->work)
                ondemand_object_worker
                 cachefiles_ondemand_init_object(A)
                  cachefiles_ondemand_send_req(OPEN)
                    // get msg_id 6
                    wait_for_completion(&req_A->done)
cachefiles_ondemand_daemon_read
 // read msg_id 6 req_A
 cachefiles_ondemand_get_fd
 copy_to_user
                                // Malicious completion msg_id 6
                                copen 6,-1
                                cachefiles_ondemand_copen
                                 complete(&req_A->done)
                                 // will not set the object to close
                                 // because ondemand_id && fd is valid.

                // ondemand_object_worker() is done
                // but the object is still reopening.

                                // new open req_B
                                cachefiles_ondemand_init_object(B)
                                 cachefiles_ondemand_send_req(OPEN)
                                 // reuse msg_id 6
process_open_req
 copen 6,A.size
 // The expected failed copen was executed successfully

Expect copen to fail, and when it does, it closes fd, which sets the
object to close, and then close triggers reopen again. However, due to
msg_id reuse resulting in a successful copen, the anonymous fd is not
closed until the daemon exits. Therefore read requests waiting for reopen
to complete may trigger hung task.

To avoid this issue, allocate the msg_id cyclically to avoid reusing the
msg_id for a very short duration of time.

Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/cachefiles/internal.h |  1 +
 fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 8ecd296cc1c4..9200c00f3e98 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -128,6 +128,7 @@ struct cachefiles_cache {
 	unsigned long			req_id_next;
 	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
 	u32				ondemand_id_next;
+	u32				msg_id_next;
 };
 
 static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index f6440b3e7368..b10952f77472 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 		smp_mb();
 
 		if (opcode == CACHEFILES_OP_CLOSE &&
-			!cachefiles_ondemand_object_is_open(object)) {
+		    !cachefiles_ondemand_object_is_open(object)) {
 			WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
 			xas_unlock(&xas);
 			ret = -EIO;
 			goto out;
 		}
 
-		xas.xa_index = 0;
+		/*
+		 * Cyclically find a free xas to avoid msg_id reuse that would
+		 * cause the daemon to successfully copen a stale msg_id.
+		 */
+		xas.xa_index = cache->msg_id_next;
 		xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
+		if (xas.xa_node == XAS_RESTART) {
+			xas.xa_index = 0;
+			xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
+		}
 		if (xas.xa_node == XAS_RESTART)
 			xas_set_err(&xas, -EBUSY);
+
 		xas_store(&xas, req);
-		xas_clear_mark(&xas, XA_FREE_MARK);
-		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
+		if (xas_valid(&xas)) {
+			cache->msg_id_next = xas.xa_index + 1;
+			xas_clear_mark(&xas, XA_FREE_MARK);
+			xas_set_mark(&xas, CACHEFILES_REQ_NEW);
+		}
 		xas_unlock(&xas);
 	} while (xas_nomem(&xas, GFP_KERNEL));
 
-- 
2.39.2

[PATCH v2 5/5] cachefiles: add missing lock protection when polling

[libaokun]

From: Jingbo Xu <jefflexu@linux.alibaba.com>

Add missing lock protection in poll routine when iterating xarray,
otherwise:

Even with RCU read lock held, only the slot of the radix tree is
ensured to be pinned there, while the data structure (e.g. struct
cachefiles_req) stored in the slot has no such guarantee.  The poll
routine will iterate the radix tree and dereference cachefiles_req
accordingly.  Thus RCU read lock is not adequate in this case and
spinlock is needed here.

Fixes: b817e22b2e91 ("cachefiles: narrow the scope of triggering EPOLLIN events in ondemand mode")
Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com>
Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/cachefiles/daemon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index 6465e2574230..73ed2323282a 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -365,14 +365,14 @@ static __poll_t cachefiles_daemon_poll(struct file *file,
 
 	if (cachefiles_in_ondemand_mode(cache)) {
 		if (!xa_empty(&cache->reqs)) {
-			rcu_read_lock();
+			xas_lock(&xas);
 			xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) {
 				if (!cachefiles_ondemand_is_reopening_read(req)) {
 					mask |= EPOLLIN;
 					break;
 				}
 			}
-			rcu_read_unlock();
+			xas_unlock(&xas);
 		}
 	} else {
 		if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))
-- 
2.39.2

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Jeff Layton]

On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> Reusing the msg_id after a maliciously completed reopen request may cause
> a read request to remain unprocessed and result in a hung, as shown below:
> 
>        t1       |      t2       |      t3
> -------------------------------------------------
> cachefiles_ondemand_select_req
>  cachefiles_ondemand_object_is_close(A)
>  cachefiles_ondemand_set_object_reopening(A)
>  queue_work(fscache_object_wq, &info->work)
>                 ondemand_object_worker
>                  cachefiles_ondemand_init_object(A)
>                   cachefiles_ondemand_send_req(OPEN)
>                     // get msg_id 6
>                     wait_for_completion(&req_A->done)
> cachefiles_ondemand_daemon_read
>  // read msg_id 6 req_A
>  cachefiles_ondemand_get_fd
>  copy_to_user
>                                 // Malicious completion msg_id 6
>                                 copen 6,-1
>                                 cachefiles_ondemand_copen
>                                  complete(&req_A->done)
>                                  // will not set the object to close
>                                  // because ondemand_id && fd is valid.
> 
>                 // ondemand_object_worker() is done
>                 // but the object is still reopening.
> 
>                                 // new open req_B
>                                 cachefiles_ondemand_init_object(B)
>                                  cachefiles_ondemand_send_req(OPEN)
>                                  // reuse msg_id 6
> process_open_req
>  copen 6,A.size
>  // The expected failed copen was executed successfully
> 
> Expect copen to fail, and when it does, it closes fd, which sets the
> object to close, and then close triggers reopen again. However, due to
> msg_id reuse resulting in a successful copen, the anonymous fd is not
> closed until the daemon exits. Therefore read requests waiting for reopen
> to complete may trigger hung task.
> 
> To avoid this issue, allocate the msg_id cyclically to avoid reusing the
> msg_id for a very short duration of time.
> 
> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  fs/cachefiles/internal.h |  1 +
>  fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index 8ecd296cc1c4..9200c00f3e98 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>  	unsigned long			req_id_next;
>  	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
>  	u32				ondemand_id_next;
> +	u32				msg_id_next;
>  };
>  
>  static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index f6440b3e7368..b10952f77472 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>  		smp_mb();
>  
>  		if (opcode == CACHEFILES_OP_CLOSE &&
> -			!cachefiles_ondemand_object_is_open(object)) {
> +		    !cachefiles_ondemand_object_is_open(object)) {
>  			WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>  			xas_unlock(&xas);
>  			ret = -EIO;
>  			goto out;
>  		}
>  
> -		xas.xa_index = 0;
> +		/*
> +		 * Cyclically find a free xas to avoid msg_id reuse that would
> +		 * cause the daemon to successfully copen a stale msg_id.
> +		 */
> +		xas.xa_index = cache->msg_id_next;
>  		xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
> +		if (xas.xa_node == XAS_RESTART) {
> +			xas.xa_index = 0;
> +			xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
> +		}
>  		if (xas.xa_node == XAS_RESTART)
>  			xas_set_err(&xas, -EBUSY);
> +
>  		xas_store(&xas, req);
> -		xas_clear_mark(&xas, XA_FREE_MARK);
> -		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> +		if (xas_valid(&xas)) {
> +			cache->msg_id_next = xas.xa_index + 1;

If you have a long-standing stuck request, could this counter wrap
around and you still end up with reuse? Maybe this should be using
ida_alloc/free instead, which would prevent that too?



> +			xas_clear_mark(&xas, XA_FREE_MARK);
> +			xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> +		}
>  		xas_unlock(&xas);
>  	} while (xas_nomem(&xas, GFP_KERNEL));
>  

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Baokun Li]

Hi Jeff,

Thank you very much for your review!

On 2024/5/19 19:11, Jeff Layton wrote:
> On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> Reusing the msg_id after a maliciously completed reopen request may cause
>> a read request to remain unprocessed and result in a hung, as shown below:
>>
>>         t1       |      t2       |      t3
>> -------------------------------------------------
>> cachefiles_ondemand_select_req
>>   cachefiles_ondemand_object_is_close(A)
>>   cachefiles_ondemand_set_object_reopening(A)
>>   queue_work(fscache_object_wq, &info->work)
>>                  ondemand_object_worker
>>                   cachefiles_ondemand_init_object(A)
>>                    cachefiles_ondemand_send_req(OPEN)
>>                      // get msg_id 6
>>                      wait_for_completion(&req_A->done)
>> cachefiles_ondemand_daemon_read
>>   // read msg_id 6 req_A
>>   cachefiles_ondemand_get_fd
>>   copy_to_user
>>                                  // Malicious completion msg_id 6
>>                                  copen 6,-1
>>                                  cachefiles_ondemand_copen
>>                                   complete(&req_A->done)
>>                                   // will not set the object to close
>>                                   // because ondemand_id && fd is valid.
>>
>>                  // ondemand_object_worker() is done
>>                  // but the object is still reopening.
>>
>>                                  // new open req_B
>>                                  cachefiles_ondemand_init_object(B)
>>                                   cachefiles_ondemand_send_req(OPEN)
>>                                   // reuse msg_id 6
>> process_open_req
>>   copen 6,A.size
>>   // The expected failed copen was executed successfully
>>
>> Expect copen to fail, and when it does, it closes fd, which sets the
>> object to close, and then close triggers reopen again. However, due to
>> msg_id reuse resulting in a successful copen, the anonymous fd is not
>> closed until the daemon exits. Therefore read requests waiting for reopen
>> to complete may trigger hung task.
>>
>> To avoid this issue, allocate the msg_id cyclically to avoid reusing the
>> msg_id for a very short duration of time.
>>
>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   fs/cachefiles/internal.h |  1 +
>>   fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>>   2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>> index 8ecd296cc1c4..9200c00f3e98 100644
>> --- a/fs/cachefiles/internal.h
>> +++ b/fs/cachefiles/internal.h
>> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>>   	unsigned long			req_id_next;
>>   	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
>>   	u32				ondemand_id_next;
>> +	u32				msg_id_next;
>>   };
>>   
>>   static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index f6440b3e7368..b10952f77472 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>   		smp_mb();
>>   
>>   		if (opcode == CACHEFILES_OP_CLOSE &&
>> -			!cachefiles_ondemand_object_is_open(object)) {
>> +		    !cachefiles_ondemand_object_is_open(object)) {
>>   			WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>>   			xas_unlock(&xas);
>>   			ret = -EIO;
>>   			goto out;
>>   		}
>>   
>> -		xas.xa_index = 0;
>> +		/*
>> +		 * Cyclically find a free xas to avoid msg_id reuse that would
>> +		 * cause the daemon to successfully copen a stale msg_id.
>> +		 */
>> +		xas.xa_index = cache->msg_id_next;
>>   		xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
>> +		if (xas.xa_node == XAS_RESTART) {
>> +			xas.xa_index = 0;
>> +			xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
>> +		}
>>   		if (xas.xa_node == XAS_RESTART)
>>   			xas_set_err(&xas, -EBUSY);
>> +
>>   		xas_store(&xas, req);
>> -		xas_clear_mark(&xas, XA_FREE_MARK);
>> -		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>> +		if (xas_valid(&xas)) {
>> +			cache->msg_id_next = xas.xa_index + 1;
> If you have a long-standing stuck request, could this counter wrap
> around and you still end up with reuse?
Yes, msg_id_next is declared to be of type u32 in the hope that when
xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
the xarry being too deep.

If msg_id_next is equal to the id of a long-standing stuck request
after the wrap-around, it is true that the reuse in the above problem
may also occur.

But I feel that a long stuck request is problematic in itself, it means
that after we have sent 4294967295 requests, the first one has not
been processed yet, and even if we send a million requests per
second, this one hasn't been completed for more than an hour.

We have a keep-alive process that pulls the daemon back up as
soon as it exits, and there is a timeout mechanism for requests in
the daemon to prevent the kernel from waiting for long periods
of time. In other words, we should avoid the situation where
a request is stuck for a long period of time.

If you think UINT_MAX is not enough, perhaps we could raise
the maximum value of msg_id_next to ULONG_MAX?
> Maybe this should be using
> ida_alloc/free instead, which would prevent that too?
>
The id reuse here is that the kernel has finished the open request
req_A and freed its id_A and used it again when sending the open
request req_B, but the daemon is still working on req_A, so the
copen id_A succeeds but operates on req_B.

The id that is being used by the kernel will not be allocated here
so it seems that ida _alloc/free does not prevent reuse either,
could you elaborate a bit more how this works?

>
>> +			xas_clear_mark(&xas, XA_FREE_MARK);
>> +			xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>> +		}
>>   		xas_unlock(&xas);
>>   	} while (xas_nomem(&xas, GFP_KERNEL));
>>   

Thanks again!

-- 
With Best Regards,
Baokun Li

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Jeff Layton]

On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
> Hi Jeff,
> 
> Thank you very much for your review!
> 
> On 2024/5/19 19:11, Jeff Layton wrote:
> > On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
> > > From: Baokun Li <libaokun1@huawei.com>
> > > 
> > > Reusing the msg_id after a maliciously completed reopen request may cause
> > > a read request to remain unprocessed and result in a hung, as shown below:
> > > 
> > >         t1       |      t2       |      t3
> > > -------------------------------------------------
> > > cachefiles_ondemand_select_req
> > >   cachefiles_ondemand_object_is_close(A)
> > >   cachefiles_ondemand_set_object_reopening(A)
> > >   queue_work(fscache_object_wq, &info->work)
> > >                  ondemand_object_worker
> > >                   cachefiles_ondemand_init_object(A)
> > >                    cachefiles_ondemand_send_req(OPEN)
> > >                      // get msg_id 6
> > >                      wait_for_completion(&req_A->done)
> > > cachefiles_ondemand_daemon_read
> > >   // read msg_id 6 req_A
> > >   cachefiles_ondemand_get_fd
> > >   copy_to_user
> > >                                  // Malicious completion msg_id 6
> > >                                  copen 6,-1
> > >                                  cachefiles_ondemand_copen
> > >                                   complete(&req_A->done)
> > >                                   // will not set the object to close
> > >                                   // because ondemand_id && fd is valid.
> > > 
> > >                  // ondemand_object_worker() is done
> > >                  // but the object is still reopening.
> > > 
> > >                                  // new open req_B
> > >                                  cachefiles_ondemand_init_object(B)
> > >                                   cachefiles_ondemand_send_req(OPEN)
> > >                                   // reuse msg_id 6
> > > process_open_req
> > >   copen 6,A.size
> > >   // The expected failed copen was executed successfully
> > > 
> > > Expect copen to fail, and when it does, it closes fd, which sets the
> > > object to close, and then close triggers reopen again. However, due to
> > > msg_id reuse resulting in a successful copen, the anonymous fd is not
> > > closed until the daemon exits. Therefore read requests waiting for reopen
> > > to complete may trigger hung task.
> > > 
> > > To avoid this issue, allocate the msg_id cyclically to avoid reusing the
> > > msg_id for a very short duration of time.
> > > 
> > > Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
> > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > ---
> > >   fs/cachefiles/internal.h |  1 +
> > >   fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
> > >   2 files changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> > > index 8ecd296cc1c4..9200c00f3e98 100644
> > > --- a/fs/cachefiles/internal.h
> > > +++ b/fs/cachefiles/internal.h
> > > @@ -128,6 +128,7 @@ struct cachefiles_cache {
> > >   	unsigned long			req_id_next;
> > >   	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
> > >   	u32				ondemand_id_next;
> > > +	u32				msg_id_next;
> > >   };
> > >   
> > >   static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
> > > diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> > > index f6440b3e7368..b10952f77472 100644
> > > --- a/fs/cachefiles/ondemand.c
> > > +++ b/fs/cachefiles/ondemand.c
> > > @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
> > >   		smp_mb();
> > >   
> > >   		if (opcode == CACHEFILES_OP_CLOSE &&
> > > -			!cachefiles_ondemand_object_is_open(object)) {
> > > +		    !cachefiles_ondemand_object_is_open(object)) {
> > >   			WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
> > >   			xas_unlock(&xas);
> > >   			ret = -EIO;
> > >   			goto out;
> > >   		}
> > >   
> > > -		xas.xa_index = 0;
> > > +		/*
> > > +		 * Cyclically find a free xas to avoid msg_id reuse that would
> > > +		 * cause the daemon to successfully copen a stale msg_id.
> > > +		 */
> > > +		xas.xa_index = cache->msg_id_next;
> > >   		xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
> > > +		if (xas.xa_node == XAS_RESTART) {
> > > +			xas.xa_index = 0;
> > > +			xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
> > > +		}
> > >   		if (xas.xa_node == XAS_RESTART)
> > >   			xas_set_err(&xas, -EBUSY);
> > > +
> > >   		xas_store(&xas, req);
> > > -		xas_clear_mark(&xas, XA_FREE_MARK);
> > > -		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> > > +		if (xas_valid(&xas)) {
> > > +			cache->msg_id_next = xas.xa_index + 1;
> > If you have a long-standing stuck request, could this counter wrap
> > around and you still end up with reuse?
> Yes, msg_id_next is declared to be of type u32 in the hope that when
> xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
> goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
> the xarry being too deep.
> 
> If msg_id_next is equal to the id of a long-standing stuck request
> after the wrap-around, it is true that the reuse in the above problem
> may also occur.
> 
> But I feel that a long stuck request is problematic in itself, it means
> that after we have sent 4294967295 requests, the first one has not
> been processed yet, and even if we send a million requests per
> second, this one hasn't been completed for more than an hour.
> 
> We have a keep-alive process that pulls the daemon back up as
> soon as it exits, and there is a timeout mechanism for requests in
> the daemon to prevent the kernel from waiting for long periods
> of time. In other words, we should avoid the situation where
> a request is stuck for a long period of time.
> 
> If you think UINT_MAX is not enough, perhaps we could raise
> the maximum value of msg_id_next to ULONG_MAX?
> > Maybe this should be using
> > ida_alloc/free instead, which would prevent that too?
> > 
> The id reuse here is that the kernel has finished the open request
> req_A and freed its id_A and used it again when sending the open
> request req_B, but the daemon is still working on req_A, so the
> copen id_A succeeds but operates on req_B.
> 
> The id that is being used by the kernel will not be allocated here
> so it seems that ida _alloc/free does not prevent reuse either,
> could you elaborate a bit more how this works?
> 

ida_alloc and free absolutely prevent reuse while the id is in use.
That's sort of the point of those functions. Basically it uses a set of
bitmaps in an xarray to track which IDs are in use, so ida_alloc only
hands out values which are not in use. See the comments over
ida_alloc_range() in lib/idr.c.


> > 
> > > +			xas_clear_mark(&xas, XA_FREE_MARK);
> > > +			xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> > > +		}
> > >   		xas_unlock(&xas);
> > >   	} while (xas_nomem(&xas, GFP_KERNEL));
> > >   
> 
> Thanks again!
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Baokun Li]

On 2024/5/20 18:04, Jeff Layton wrote:
> On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
>> Hi Jeff,
>>
>> Thank you very much for your review!
>>
>> On 2024/5/19 19:11, Jeff Layton wrote:
>>> On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
>>>> From: Baokun Li <libaokun1@huawei.com>
>>>>
>>>> Reusing the msg_id after a maliciously completed reopen request may cause
>>>> a read request to remain unprocessed and result in a hung, as shown below:
>>>>
>>>>          t1       |      t2       |      t3
>>>> -------------------------------------------------
>>>> cachefiles_ondemand_select_req
>>>>    cachefiles_ondemand_object_is_close(A)
>>>>    cachefiles_ondemand_set_object_reopening(A)
>>>>    queue_work(fscache_object_wq, &info->work)
>>>>                   ondemand_object_worker
>>>>                    cachefiles_ondemand_init_object(A)
>>>>                     cachefiles_ondemand_send_req(OPEN)
>>>>                       // get msg_id 6
>>>>                       wait_for_completion(&req_A->done)
>>>> cachefiles_ondemand_daemon_read
>>>>    // read msg_id 6 req_A
>>>>    cachefiles_ondemand_get_fd
>>>>    copy_to_user
>>>>                                   // Malicious completion msg_id 6
>>>>                                   copen 6,-1
>>>>                                   cachefiles_ondemand_copen
>>>>                                    complete(&req_A->done)
>>>>                                    // will not set the object to close
>>>>                                    // because ondemand_id && fd is valid.
>>>>
>>>>                   // ondemand_object_worker() is done
>>>>                   // but the object is still reopening.
>>>>
>>>>                                   // new open req_B
>>>>                                   cachefiles_ondemand_init_object(B)
>>>>                                    cachefiles_ondemand_send_req(OPEN)
>>>>                                    // reuse msg_id 6
>>>> process_open_req
>>>>    copen 6,A.size
>>>>    // The expected failed copen was executed successfully
>>>>
>>>> Expect copen to fail, and when it does, it closes fd, which sets the
>>>> object to close, and then close triggers reopen again. However, due to
>>>> msg_id reuse resulting in a successful copen, the anonymous fd is not
>>>> closed until the daemon exits. Therefore read requests waiting for reopen
>>>> to complete may trigger hung task.
>>>>
>>>> To avoid this issue, allocate the msg_id cyclically to avoid reusing the
>>>> msg_id for a very short duration of time.
>>>>
>>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>> ---
>>>>    fs/cachefiles/internal.h |  1 +
>>>>    fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>>>>    2 files changed, 17 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>>>> index 8ecd296cc1c4..9200c00f3e98 100644
>>>> --- a/fs/cachefiles/internal.h
>>>> +++ b/fs/cachefiles/internal.h
>>>> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>>>>    	unsigned long			req_id_next;
>>>>    	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
>>>>    	u32				ondemand_id_next;
>>>> +	u32				msg_id_next;
>>>>    };
>>>>    
>>>>    static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
>>>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>>>> index f6440b3e7368..b10952f77472 100644
>>>> --- a/fs/cachefiles/ondemand.c
>>>> +++ b/fs/cachefiles/ondemand.c
>>>> @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>>>    		smp_mb();
>>>>    
>>>>    		if (opcode == CACHEFILES_OP_CLOSE &&
>>>> -			!cachefiles_ondemand_object_is_open(object)) {
>>>> +		    !cachefiles_ondemand_object_is_open(object)) {
>>>>    			WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>>>>    			xas_unlock(&xas);
>>>>    			ret = -EIO;
>>>>    			goto out;
>>>>    		}
>>>>    
>>>> -		xas.xa_index = 0;
>>>> +		/*
>>>> +		 * Cyclically find a free xas to avoid msg_id reuse that would
>>>> +		 * cause the daemon to successfully copen a stale msg_id.
>>>> +		 */
>>>> +		xas.xa_index = cache->msg_id_next;
>>>>    		xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
>>>> +		if (xas.xa_node == XAS_RESTART) {
>>>> +			xas.xa_index = 0;
>>>> +			xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
>>>> +		}
>>>>    		if (xas.xa_node == XAS_RESTART)
>>>>    			xas_set_err(&xas, -EBUSY);
>>>> +
>>>>    		xas_store(&xas, req);
>>>> -		xas_clear_mark(&xas, XA_FREE_MARK);
>>>> -		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>> +		if (xas_valid(&xas)) {
>>>> +			cache->msg_id_next = xas.xa_index + 1;
>>> If you have a long-standing stuck request, could this counter wrap
>>> around and you still end up with reuse?
>> Yes, msg_id_next is declared to be of type u32 in the hope that when
>> xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
>> goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
>> the xarry being too deep.
>>
>> If msg_id_next is equal to the id of a long-standing stuck request
>> after the wrap-around, it is true that the reuse in the above problem
>> may also occur.
>>
>> But I feel that a long stuck request is problematic in itself, it means
>> that after we have sent 4294967295 requests, the first one has not
>> been processed yet, and even if we send a million requests per
>> second, this one hasn't been completed for more than an hour.
>>
>> We have a keep-alive process that pulls the daemon back up as
>> soon as it exits, and there is a timeout mechanism for requests in
>> the daemon to prevent the kernel from waiting for long periods
>> of time. In other words, we should avoid the situation where
>> a request is stuck for a long period of time.
>>
>> If you think UINT_MAX is not enough, perhaps we could raise
>> the maximum value of msg_id_next to ULONG_MAX?
>>> Maybe this should be using
>>> ida_alloc/free instead, which would prevent that too?
>>>
>> The id reuse here is that the kernel has finished the open request
>> req_A and freed its id_A and used it again when sending the open
>> request req_B, but the daemon is still working on req_A, so the
>> copen id_A succeeds but operates on req_B.
>>
>> The id that is being used by the kernel will not be allocated here
>> so it seems that ida _alloc/free does not prevent reuse either,
>> could you elaborate a bit more how this works?
>>
> ida_alloc and free absolutely prevent reuse while the id is in use.
> That's sort of the point of those functions. Basically it uses a set of
> bitmaps in an xarray to track which IDs are in use, so ida_alloc only
> hands out values which are not in use. See the comments over
> ida_alloc_range() in lib/idr.c.
>
Thank you for the explanation!

The logic now provides the same guarantees as ida_alloc/free.
The "reused" id, indeed, is no longer in use in the kernel, but it is still
in use in the userland, so a multi-threaded daemon could be handling
two different requests for the same msg_id at the same time.

Previously, the logic for allocating msg_ids was to start at 0 and look
for a free xas.index, so it was possible for an id to be allocated to a
new request just as the id was being freed.

With the change to cyclic allocation, the kernel will not use the same
id again until INT_MAX requests have been sent, and during the time
it takes to send requests, the daemon has enough time to process
requests whose ids are still in use by the daemon, but have already
been freed in the kernel.

Regards,
Baokun
>>>> +			xas_clear_mark(&xas, XA_FREE_MARK);
>>>> +			xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>> +		}
>>>>    		xas_unlock(&xas);
>>>>    	} while (xas_nomem(&xas, GFP_KERNEL));
>>>>    
>>>>

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Gao Xiang]

On 2024/5/20 20:42, Baokun Li wrote:
> On 2024/5/20 18:04, Jeff Layton wrote:
>> On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
>>> Hi Jeff,
>>>
>>> Thank you very much for your review!
>>>
>>> On 2024/5/19 19:11, Jeff Layton wrote:
>>>> On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
>>>>> From: Baokun Li <libaokun1@huawei.com>
>>>>>
>>>>> Reusing the msg_id after a maliciously completed reopen request may cause
>>>>> a read request to remain unprocessed and result in a hung, as shown below:
>>>>>
>>>>>          t1       |      t2       |      t3
>>>>> -------------------------------------------------
>>>>> cachefiles_ondemand_select_req
>>>>>    cachefiles_ondemand_object_is_close(A)
>>>>>    cachefiles_ondemand_set_object_reopening(A)
>>>>>    queue_work(fscache_object_wq, &info->work)
>>>>>                   ondemand_object_worker
>>>>>                    cachefiles_ondemand_init_object(A)
>>>>>                     cachefiles_ondemand_send_req(OPEN)
>>>>>                       // get msg_id 6
>>>>>                       wait_for_completion(&req_A->done)
>>>>> cachefiles_ondemand_daemon_read
>>>>>    // read msg_id 6 req_A
>>>>>    cachefiles_ondemand_get_fd
>>>>>    copy_to_user
>>>>>                                   // Malicious completion msg_id 6
>>>>>                                   copen 6,-1
>>>>>                                   cachefiles_ondemand_copen
>>>>>                                    complete(&req_A->done)
>>>>>                                    // will not set the object to close
>>>>>                                    // because ondemand_id && fd is valid.
>>>>>
>>>>>                   // ondemand_object_worker() is done
>>>>>                   // but the object is still reopening.
>>>>>
>>>>>                                   // new open req_B
>>>>>                                   cachefiles_ondemand_init_object(B)
>>>>>                                    cachefiles_ondemand_send_req(OPEN)
>>>>>                                    // reuse msg_id 6
>>>>> process_open_req
>>>>>    copen 6,A.size
>>>>>    // The expected failed copen was executed successfully
>>>>>
>>>>> Expect copen to fail, and when it does, it closes fd, which sets the
>>>>> object to close, and then close triggers reopen again. However, due to
>>>>> msg_id reuse resulting in a successful copen, the anonymous fd is not
>>>>> closed until the daemon exits. Therefore read requests waiting for reopen
>>>>> to complete may trigger hung task.
>>>>>
>>>>> To avoid this issue, allocate the msg_id cyclically to avoid reusing the
>>>>> msg_id for a very short duration of time.
>>>>>
>>>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
>>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>>> ---
>>>>>    fs/cachefiles/internal.h |  1 +
>>>>>    fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>>>>>    2 files changed, 17 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>>>>> index 8ecd296cc1c4..9200c00f3e98 100644
>>>>> --- a/fs/cachefiles/internal.h
>>>>> +++ b/fs/cachefiles/internal.h
>>>>> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>>>>>        unsigned long            req_id_next;
>>>>>        struct xarray            ondemand_ids;    /* xarray for ondemand_id allocation */
>>>>>        u32                ondemand_id_next;
>>>>> +    u32                msg_id_next;
>>>>>    };
>>>>>    static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
>>>>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>>>>> index f6440b3e7368..b10952f77472 100644
>>>>> --- a/fs/cachefiles/ondemand.c
>>>>> +++ b/fs/cachefiles/ondemand.c
>>>>> @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>>>>            smp_mb();
>>>>>            if (opcode == CACHEFILES_OP_CLOSE &&
>>>>> -            !cachefiles_ondemand_object_is_open(object)) {
>>>>> +            !cachefiles_ondemand_object_is_open(object)) {
>>>>>                WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>>>>>                xas_unlock(&xas);
>>>>>                ret = -EIO;
>>>>>                goto out;
>>>>>            }
>>>>> -        xas.xa_index = 0;
>>>>> +        /*
>>>>> +         * Cyclically find a free xas to avoid msg_id reuse that would
>>>>> +         * cause the daemon to successfully copen a stale msg_id.
>>>>> +         */
>>>>> +        xas.xa_index = cache->msg_id_next;
>>>>>            xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
>>>>> +        if (xas.xa_node == XAS_RESTART) {
>>>>> +            xas.xa_index = 0;
>>>>> +            xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
>>>>> +        }
>>>>>            if (xas.xa_node == XAS_RESTART)
>>>>>                xas_set_err(&xas, -EBUSY);
>>>>> +
>>>>>            xas_store(&xas, req);
>>>>> -        xas_clear_mark(&xas, XA_FREE_MARK);
>>>>> -        xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>> +        if (xas_valid(&xas)) {
>>>>> +            cache->msg_id_next = xas.xa_index + 1;
>>>> If you have a long-standing stuck request, could this counter wrap
>>>> around and you still end up with reuse?
>>> Yes, msg_id_next is declared to be of type u32 in the hope that when
>>> xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
>>> goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
>>> the xarry being too deep.
>>>
>>> If msg_id_next is equal to the id of a long-standing stuck request
>>> after the wrap-around, it is true that the reuse in the above problem
>>> may also occur.
>>>
>>> But I feel that a long stuck request is problematic in itself, it means
>>> that after we have sent 4294967295 requests, the first one has not
>>> been processed yet, and even if we send a million requests per
>>> second, this one hasn't been completed for more than an hour.
>>>
>>> We have a keep-alive process that pulls the daemon back up as
>>> soon as it exits, and there is a timeout mechanism for requests in
>>> the daemon to prevent the kernel from waiting for long periods
>>> of time. In other words, we should avoid the situation where
>>> a request is stuck for a long period of time.
>>>
>>> If you think UINT_MAX is not enough, perhaps we could raise
>>> the maximum value of msg_id_next to ULONG_MAX?
>>>> Maybe this should be using
>>>> ida_alloc/free instead, which would prevent that too?
>>>>
>>> The id reuse here is that the kernel has finished the open request
>>> req_A and freed its id_A and used it again when sending the open
>>> request req_B, but the daemon is still working on req_A, so the
>>> copen id_A succeeds but operates on req_B.
>>>
>>> The id that is being used by the kernel will not be allocated here
>>> so it seems that ida _alloc/free does not prevent reuse either,
>>> could you elaborate a bit more how this works?
>>>
>> ida_alloc and free absolutely prevent reuse while the id is in use.
>> That's sort of the point of those functions. Basically it uses a set of
>> bitmaps in an xarray to track which IDs are in use, so ida_alloc only
>> hands out values which are not in use. See the comments over
>> ida_alloc_range() in lib/idr.c.
>>
> Thank you for the explanation!
> 
> The logic now provides the same guarantees as ida_alloc/free.
> The "reused" id, indeed, is no longer in use in the kernel, but it is still
> in use in the userland, so a multi-threaded daemon could be handling
> two different requests for the same msg_id at the same time.
> 
> Previously, the logic for allocating msg_ids was to start at 0 and look
> for a free xas.index, so it was possible for an id to be allocated to a
> new request just as the id was being freed.
> 
> With the change to cyclic allocation, the kernel will not use the same
> id again until INT_MAX requests have been sent, and during the time
> it takes to send requests, the daemon has enough time to process
> requests whose ids are still in use by the daemon, but have already
> been freed in the kernel.

Again, If I understand correctly, I think the main point
here is

wait_for_completion(&req_A->done)

which could hang due to some malicious deamon.  But I think it
should be switched to wait_for_completion_killable() instead.
It's up to users to kill the mount instance if there is a
malicious user daemon.

So in that case, hung task will not be triggered anymore, and
you don't need to care about cyclic allocation too.

Thanks,
Gao Xiang

> 
> Regards,
> Baokun
>>>>> +            xas_clear_mark(&xas, XA_FREE_MARK);
>>>>> +            xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>> +        }
>>>>>            xas_unlock(&xas);
>>>>>        } while (xas_nomem(&xas, GFP_KERNEL));
>>>>>

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Jeff Layton]

On Mon, 2024-05-20 at 20:42 +0800, Baokun Li wrote:
> On 2024/5/20 18:04, Jeff Layton wrote:
> > On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
> > > Hi Jeff,
> > > 
> > > Thank you very much for your review!
> > > 
> > > On 2024/5/19 19:11, Jeff Layton wrote:
> > > > On Wed, 2024-05-15 at 20:51 +0800,
> > > > libaokun@huaweicloud.com wrote:
> > > > > From: Baokun Li <libaokun1@huawei.com>
> > > > > 
> > > > > Reusing the msg_id after a maliciously completed reopen
> > > > > request may cause
> > > > > a read request to remain unprocessed and result in a hung, as
> > > > > shown below:
> > > > > 
> > > > >          t1       |      t2       |      t3
> > > > > -------------------------------------------------
> > > > > cachefiles_ondemand_select_req
> > > > >    cachefiles_ondemand_object_is_close(A)
> > > > >    cachefiles_ondemand_set_object_reopening(A)
> > > > >    queue_work(fscache_object_wq, &info->work)
> > > > >                   ondemand_object_worker
> > > > >                    cachefiles_ondemand_init_object(A)
> > > > >                     cachefiles_ondemand_send_req(OPEN)
> > > > >                       // get msg_id 6
> > > > >                       wait_for_completion(&req_A->done)
> > > > > cachefiles_ondemand_daemon_read
> > > > >    // read msg_id 6 req_A
> > > > >    cachefiles_ondemand_get_fd
> > > > >    copy_to_user
> > > > >                                   // Malicious completion
> > > > > msg_id 6
> > > > >                                   copen 6,-1
> > > > >                                   cachefiles_ondemand_copen
> > > > >                                    complete(&req_A->done)
> > > > >                                    // will not set the object
> > > > > to close
> > > > >                                    // because ondemand_id &&
> > > > > fd is valid.
> > > > > 
> > > > >                   // ondemand_object_worker() is done
> > > > >                   // but the object is still reopening.
> > > > > 
> > > > >                                   // new open req_B
> > > > >                                  
> > > > > cachefiles_ondemand_init_object(B)
> > > > >                                   
> > > > > cachefiles_ondemand_send_req(OPEN)
> > > > >                                    // reuse msg_id 6
> > > > > process_open_req
> > > > >    copen 6,A.size
> > > > >    // The expected failed copen was executed successfully
> > > > > 
> > > > > Expect copen to fail, and when it does, it closes fd, which
> > > > > sets the
> > > > > object to close, and then close triggers reopen again.
> > > > > However, due to
> > > > > msg_id reuse resulting in a successful copen, the anonymous
> > > > > fd is not
> > > > > closed until the daemon exits. Therefore read requests
> > > > > waiting for reopen
> > > > > to complete may trigger hung task.
> > > > > 
> > > > > To avoid this issue, allocate the msg_id cyclically to avoid
> > > > > reusing the
> > > > > msg_id for a very short duration of time.
> > > > > 
> > > > > Fixes: c8383054506c ("cachefiles: notify the user daemon when
> > > > > looking up cookie")
> > > > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > > > ---
> > > > >    fs/cachefiles/internal.h |  1 +
> > > > >    fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
> > > > >    2 files changed, 17 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/fs/cachefiles/internal.h
> > > > > b/fs/cachefiles/internal.h
> > > > > index 8ecd296cc1c4..9200c00f3e98 100644
> > > > > --- a/fs/cachefiles/internal.h
> > > > > +++ b/fs/cachefiles/internal.h
> > > > > @@ -128,6 +128,7 @@ struct cachefiles_cache {
> > > > >    	unsigned long			req_id_next;
> > > > >    	struct xarray			ondemand_ids;	/*
> > > > > xarray for ondemand_id allocation */
> > > > >    	u32				ondemand_id_next;
> > > > > +	u32				msg_id_next;
> > > > >    };
> > > > >    
> > > > >    static inline bool cachefiles_in_ondemand_mode(struct
> > > > > cachefiles_cache *cache)
> > > > > diff --git a/fs/cachefiles/ondemand.c
> > > > > b/fs/cachefiles/ondemand.c
> > > > > index f6440b3e7368..b10952f77472 100644
> > > > > --- a/fs/cachefiles/ondemand.c
> > > > > +++ b/fs/cachefiles/ondemand.c
> > > > > @@ -433,20 +433,32 @@ static int
> > > > > cachefiles_ondemand_send_req(struct cachefiles_object
> > > > > *object,
> > > > >    		smp_mb();
> > > > >    
> > > > >    		if (opcode == CACHEFILES_OP_CLOSE &&
> > > > > -
> > > > > 			!cachefiles_ondemand_object_is_open(object)) {
> > > > > +		   
> > > > > !cachefiles_ondemand_object_is_open(object)) {
> > > > >    			WARN_ON_ONCE(object->ondemand-
> > > > > >ondemand_id == 0);
> > > > >    			xas_unlock(&xas);
> > > > >    			ret = -EIO;
> > > > >    			goto out;
> > > > >    		}
> > > > >    
> > > > > -		xas.xa_index = 0;
> > > > > +		/*
> > > > > +		 * Cyclically find a free xas to avoid
> > > > > msg_id reuse that would
> > > > > +		 * cause the daemon to successfully copen a
> > > > > stale msg_id.
> > > > > +		 */
> > > > > +		xas.xa_index = cache->msg_id_next;
> > > > >    		xas_find_marked(&xas, UINT_MAX,
> > > > > XA_FREE_MARK);
> > > > > +		if (xas.xa_node == XAS_RESTART) {
> > > > > +			xas.xa_index = 0;
> > > > > +			xas_find_marked(&xas, cache-
> > > > > >msg_id_next - 1, XA_FREE_MARK);
> > > > > +		}
> > > > >    		if (xas.xa_node == XAS_RESTART)
> > > > >    			xas_set_err(&xas, -EBUSY);
> > > > > +
> > > > >    		xas_store(&xas, req);
> > > > > -		xas_clear_mark(&xas, XA_FREE_MARK);
> > > > > -		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> > > > > +		if (xas_valid(&xas)) {
> > > > > +			cache->msg_id_next = xas.xa_index +
> > > > > 1;
> > > > If you have a long-standing stuck request, could this counter
> > > > wrap
> > > > around and you still end up with reuse?
> > > Yes, msg_id_next is declared to be of type u32 in the hope that
> > > when
> > > xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
> > > goes to zero. Limiting xa_index to no more than UINT_MAX is to
> > > avoid
> > > the xarry being too deep.
> > > 
> > > If msg_id_next is equal to the id of a long-standing stuck
> > > request
> > > after the wrap-around, it is true that the reuse in the above
> > > problem
> > > may also occur.
> > > 
> > > But I feel that a long stuck request is problematic in itself, it
> > > means
> > > that after we have sent 4294967295 requests, the first one has
> > > not
> > > been processed yet, and even if we send a million requests per
> > > second, this one hasn't been completed for more than an hour.
> > > 
> > > We have a keep-alive process that pulls the daemon back up as
> > > soon as it exits, and there is a timeout mechanism for requests
> > > in
> > > the daemon to prevent the kernel from waiting for long periods
> > > of time. In other words, we should avoid the situation where
> > > a request is stuck for a long period of time.
> > > 
> > > If you think UINT_MAX is not enough, perhaps we could raise
> > > the maximum value of msg_id_next to ULONG_MAX?
> > > > Maybe this should be using
> > > > ida_alloc/free instead, which would prevent that too?
> > > > 
> > > The id reuse here is that the kernel has finished the open
> > > request
> > > req_A and freed its id_A and used it again when sending the open
> > > request req_B, but the daemon is still working on req_A, so the
> > > copen id_A succeeds but operates on req_B.
> > > 
> > > The id that is being used by the kernel will not be allocated
> > > here
> > > so it seems that ida _alloc/free does not prevent reuse either,
> > > could you elaborate a bit more how this works?
> > > 
> > ida_alloc and free absolutely prevent reuse while the id is in use.
> > That's sort of the point of those functions. Basically it uses a
> > set of
> > bitmaps in an xarray to track which IDs are in use, so ida_alloc
> > only
> > hands out values which are not in use. See the comments over
> > ida_alloc_range() in lib/idr.c.
> > 
> Thank you for the explanation!
> 
> The logic now provides the same guarantees as ida_alloc/free.
> The "reused" id, indeed, is no longer in use in the kernel, but it is
> still
> in use in the userland, so a multi-threaded daemon could be handling
> two different requests for the same msg_id at the same time.
> 
> Previously, the logic for allocating msg_ids was to start at 0 and
> look
> for a free xas.index, so it was possible for an id to be allocated to
> a
> new request just as the id was being freed.
> 
> With the change to cyclic allocation, the kernel will not use the
> same
> id again until INT_MAX requests have been sent, and during the time
> it takes to send requests, the daemon has enough time to process
> requests whose ids are still in use by the daemon, but have already
> been freed in the kernel.
> 
> 

If you're checking for collisions somewhere else, then this should be
fine:

Acked-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Baokun Li]

On 2024/5/20 20:54, Gao Xiang wrote:
>
>
> On 2024/5/20 20:42, Baokun Li wrote:
>> On 2024/5/20 18:04, Jeff Layton wrote:
>>> On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
>>>> Hi Jeff,
>>>>
>>>> Thank you very much for your review!
>>>>
>>>> On 2024/5/19 19:11, Jeff Layton wrote:
>>>>> On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
>>>>>> From: Baokun Li <libaokun1@huawei.com>
>>>>>>
>>>>>> Reusing the msg_id after a maliciously completed reopen request 
>>>>>> may cause
>>>>>> a read request to remain unprocessed and result in a hung, as 
>>>>>> shown below:
>>>>>>
>>>>>>          t1       |      t2       |      t3
>>>>>> -------------------------------------------------
>>>>>> cachefiles_ondemand_select_req
>>>>>>    cachefiles_ondemand_object_is_close(A)
>>>>>>    cachefiles_ondemand_set_object_reopening(A)
>>>>>>    queue_work(fscache_object_wq, &info->work)
>>>>>>                   ondemand_object_worker
>>>>>>                    cachefiles_ondemand_init_object(A)
>>>>>>                     cachefiles_ondemand_send_req(OPEN)
>>>>>>                       // get msg_id 6
>>>>>> wait_for_completion(&req_A->done)
>>>>>> cachefiles_ondemand_daemon_read
>>>>>>    // read msg_id 6 req_A
>>>>>>    cachefiles_ondemand_get_fd
>>>>>>    copy_to_user
>>>>>>                                   // Malicious completion msg_id 6
>>>>>>                                   copen 6,-1
>>>>>> cachefiles_ondemand_copen
>>>>>> complete(&req_A->done)
>>>>>>                                    // will not set the object to 
>>>>>> close
>>>>>>                                    // because ondemand_id && fd 
>>>>>> is valid.
>>>>>>
>>>>>>                   // ondemand_object_worker() is done
>>>>>>                   // but the object is still reopening.
>>>>>>
>>>>>>                                   // new open req_B
>>>>>> cachefiles_ondemand_init_object(B)
>>>>>> cachefiles_ondemand_send_req(OPEN)
>>>>>>                                    // reuse msg_id 6
>>>>>> process_open_req
>>>>>>    copen 6,A.size
>>>>>>    // The expected failed copen was executed successfully
>>>>>>
>>>>>> Expect copen to fail, and when it does, it closes fd, which sets the
>>>>>> object to close, and then close triggers reopen again. However, 
>>>>>> due to
>>>>>> msg_id reuse resulting in a successful copen, the anonymous fd is 
>>>>>> not
>>>>>> closed until the daemon exits. Therefore read requests waiting 
>>>>>> for reopen
>>>>>> to complete may trigger hung task.
>>>>>>
>>>>>> To avoid this issue, allocate the msg_id cyclically to avoid 
>>>>>> reusing the
>>>>>> msg_id for a very short duration of time.
>>>>>>
>>>>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when 
>>>>>> looking up cookie")
>>>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>>>> ---
>>>>>>    fs/cachefiles/internal.h |  1 +
>>>>>>    fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>>>>>>    2 files changed, 17 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>>>>>> index 8ecd296cc1c4..9200c00f3e98 100644
>>>>>> --- a/fs/cachefiles/internal.h
>>>>>> +++ b/fs/cachefiles/internal.h
>>>>>> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>>>>>>        unsigned long            req_id_next;
>>>>>>        struct xarray            ondemand_ids;    /* xarray for 
>>>>>> ondemand_id allocation */
>>>>>>        u32                ondemand_id_next;
>>>>>> +    u32                msg_id_next;
>>>>>>    };
>>>>>>    static inline bool cachefiles_in_ondemand_mode(struct 
>>>>>> cachefiles_cache *cache)
>>>>>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>>>>>> index f6440b3e7368..b10952f77472 100644
>>>>>> --- a/fs/cachefiles/ondemand.c
>>>>>> +++ b/fs/cachefiles/ondemand.c
>>>>>> @@ -433,20 +433,32 @@ static int 
>>>>>> cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>>>>>            smp_mb();
>>>>>>            if (opcode == CACHEFILES_OP_CLOSE &&
>>>>>> - !cachefiles_ondemand_object_is_open(object)) {
>>>>>> + !cachefiles_ondemand_object_is_open(object)) {
>>>>>> WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>>>>>>                xas_unlock(&xas);
>>>>>>                ret = -EIO;
>>>>>>                goto out;
>>>>>>            }
>>>>>> -        xas.xa_index = 0;
>>>>>> +        /*
>>>>>> +         * Cyclically find a free xas to avoid msg_id reuse that 
>>>>>> would
>>>>>> +         * cause the daemon to successfully copen a stale msg_id.
>>>>>> +         */
>>>>>> +        xas.xa_index = cache->msg_id_next;
>>>>>>            xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
>>>>>> +        if (xas.xa_node == XAS_RESTART) {
>>>>>> +            xas.xa_index = 0;
>>>>>> +            xas_find_marked(&xas, cache->msg_id_next - 1, 
>>>>>> XA_FREE_MARK);
>>>>>> +        }
>>>>>>            if (xas.xa_node == XAS_RESTART)
>>>>>>                xas_set_err(&xas, -EBUSY);
>>>>>> +
>>>>>>            xas_store(&xas, req);
>>>>>> -        xas_clear_mark(&xas, XA_FREE_MARK);
>>>>>> -        xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>>> +        if (xas_valid(&xas)) {
>>>>>> +            cache->msg_id_next = xas.xa_index + 1;
>>>>> If you have a long-standing stuck request, could this counter wrap
>>>>> around and you still end up with reuse?
>>>> Yes, msg_id_next is declared to be of type u32 in the hope that when
>>>> xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
>>>> goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
>>>> the xarry being too deep.
>>>>
>>>> If msg_id_next is equal to the id of a long-standing stuck request
>>>> after the wrap-around, it is true that the reuse in the above problem
>>>> may also occur.
>>>>
>>>> But I feel that a long stuck request is problematic in itself, it 
>>>> means
>>>> that after we have sent 4294967295 requests, the first one has not
>>>> been processed yet, and even if we send a million requests per
>>>> second, this one hasn't been completed for more than an hour.
>>>>
>>>> We have a keep-alive process that pulls the daemon back up as
>>>> soon as it exits, and there is a timeout mechanism for requests in
>>>> the daemon to prevent the kernel from waiting for long periods
>>>> of time. In other words, we should avoid the situation where
>>>> a request is stuck for a long period of time.
>>>>
>>>> If you think UINT_MAX is not enough, perhaps we could raise
>>>> the maximum value of msg_id_next to ULONG_MAX?
>>>>> Maybe this should be using
>>>>> ida_alloc/free instead, which would prevent that too?
>>>>>
>>>> The id reuse here is that the kernel has finished the open request
>>>> req_A and freed its id_A and used it again when sending the open
>>>> request req_B, but the daemon is still working on req_A, so the
>>>> copen id_A succeeds but operates on req_B.
>>>>
>>>> The id that is being used by the kernel will not be allocated here
>>>> so it seems that ida _alloc/free does not prevent reuse either,
>>>> could you elaborate a bit more how this works?
>>>>
>>> ida_alloc and free absolutely prevent reuse while the id is in use.
>>> That's sort of the point of those functions. Basically it uses a set of
>>> bitmaps in an xarray to track which IDs are in use, so ida_alloc only
>>> hands out values which are not in use. See the comments over
>>> ida_alloc_range() in lib/idr.c.
>>>
>> Thank you for the explanation!
>>
>> The logic now provides the same guarantees as ida_alloc/free.
>> The "reused" id, indeed, is no longer in use in the kernel, but it is 
>> still
>> in use in the userland, so a multi-threaded daemon could be handling
>> two different requests for the same msg_id at the same time.
>>
>> Previously, the logic for allocating msg_ids was to start at 0 and look
>> for a free xas.index, so it was possible for an id to be allocated to a
>> new request just as the id was being freed.
>>
>> With the change to cyclic allocation, the kernel will not use the same
>> id again until INT_MAX requests have been sent, and during the time
>> it takes to send requests, the daemon has enough time to process
>> requests whose ids are still in use by the daemon, but have already
>> been freed in the kernel.
>
> Again, If I understand correctly, I think the main point
> here is
>
> wait_for_completion(&req_A->done)
>
> which could hang due to some malicious deamon.  But I think it
> should be switched to wait_for_completion_killable() instead. *
> It's up to users to kill the mount instance if there is a
> malicious user daemon.
>
> So in that case, hung task will not be triggered anymore, and
> you don't need to care about cyclic allocation too.
>
> Thanks,
> Gao Xiang
Hi Xiang,

The problem is not as simple as you think.

If you make it killable, it just won't trigger a hung task in
cachefiles_ondemand_send_req(), and the process waiting for the
resource in question will also be hung.

* When the open/read request in the mount process gets stuck,
   the sync/drop cache will trigger a hung task panic in iterate_supers()
   as it waits for sb->umount to be unlocked.
* After umount, anonymous fd is not closed causing a hung task panic
   in fscache_hash_cookie() because of waiting for cookie unhash.
* The dentry is in a loop up state, because the read request is not being
   processed, another process looking for the same dentry is waiting for
   the previous lookup to finish, which triggers a hung task panic in
   d_alloc_parallel().

Can all this be made killable?

Thanks,
Baokun
>
>>
>> Regards,
>> Baokun
>>>>>> + xas_clear_mark(&xas, XA_FREE_MARK);
>>>>>> +            xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>>> +        }
>>>>>>            xas_unlock(&xas);
>>>>>>        } while (xas_nomem(&xas, GFP_KERNEL));
>>>>>>

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Gao Xiang]

Hi Baokun,

On 2024/5/20 21:24, Baokun Li wrote:
> On 2024/5/20 20:54, Gao Xiang wrote:
>>
>>
>> On 2024/5/20 20:42, Baokun Li wrote:
>>> On 2024/5/20 18:04, Jeff Layton wrote:
>>>> On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
>>>>> Hi Jeff,
>>>>>
>>>>> Thank you very much for your review!
>>>>>
>>>>> On 2024/5/19 19:11, Jeff Layton wrote:
>>>>>> On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
>>>>>>> From: Baokun Li <libaokun1@huawei.com>
>>>>>>>
>>>>>>> Reusing the msg_id after a maliciously completed reopen request may cause
>>>>>>> a read request to remain unprocessed and result in a hung, as shown below:
>>>>>>>
>>>>>>>          t1       |      t2       |      t3
>>>>>>> -------------------------------------------------
>>>>>>> cachefiles_ondemand_select_req
>>>>>>>    cachefiles_ondemand_object_is_close(A)
>>>>>>>    cachefiles_ondemand_set_object_reopening(A)
>>>>>>>    queue_work(fscache_object_wq, &info->work)
>>>>>>>                   ondemand_object_worker
>>>>>>>                    cachefiles_ondemand_init_object(A)
>>>>>>>                     cachefiles_ondemand_send_req(OPEN)
>>>>>>>                       // get msg_id 6
>>>>>>> wait_for_completion(&req_A->done)
>>>>>>> cachefiles_ondemand_daemon_read
>>>>>>>    // read msg_id 6 req_A
>>>>>>>    cachefiles_ondemand_get_fd
>>>>>>>    copy_to_user
>>>>>>>                                   // Malicious completion msg_id 6
>>>>>>>                                   copen 6,-1
>>>>>>> cachefiles_ondemand_copen
>>>>>>> complete(&req_A->done)
>>>>>>>                                    // will not set the object to close
>>>>>>>                                    // because ondemand_id && fd is valid.
>>>>>>>
>>>>>>>                   // ondemand_object_worker() is done
>>>>>>>                   // but the object is still reopening.
>>>>>>>
>>>>>>>                                   // new open req_B
>>>>>>> cachefiles_ondemand_init_object(B)
>>>>>>> cachefiles_ondemand_send_req(OPEN)
>>>>>>>                                    // reuse msg_id 6
>>>>>>> process_open_req
>>>>>>>    copen 6,A.size
>>>>>>>    // The expected failed copen was executed successfully
>>>>>>>
>>>>>>> Expect copen to fail, and when it does, it closes fd, which sets the
>>>>>>> object to close, and then close triggers reopen again. However, due to
>>>>>>> msg_id reuse resulting in a successful copen, the anonymous fd is not
>>>>>>> closed until the daemon exits. Therefore read requests waiting for reopen
>>>>>>> to complete may trigger hung task.
>>>>>>>
>>>>>>> To avoid this issue, allocate the msg_id cyclically to avoid reusing the
>>>>>>> msg_id for a very short duration of time.
>>>>>>>
>>>>>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
>>>>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>>>>> ---
>>>>>>>    fs/cachefiles/internal.h |  1 +
>>>>>>>    fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>>>>>>>    2 files changed, 17 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>>>>>>> index 8ecd296cc1c4..9200c00f3e98 100644
>>>>>>> --- a/fs/cachefiles/internal.h
>>>>>>> +++ b/fs/cachefiles/internal.h
>>>>>>> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>>>>>>>        unsigned long            req_id_next;
>>>>>>>        struct xarray            ondemand_ids;    /* xarray for ondemand_id allocation */
>>>>>>>        u32                ondemand_id_next;
>>>>>>> +    u32                msg_id_next;
>>>>>>>    };
>>>>>>>    static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
>>>>>>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>>>>>>> index f6440b3e7368..b10952f77472 100644
>>>>>>> --- a/fs/cachefiles/ondemand.c
>>>>>>> +++ b/fs/cachefiles/ondemand.c
>>>>>>> @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>>>>>>            smp_mb();
>>>>>>>            if (opcode == CACHEFILES_OP_CLOSE &&
>>>>>>> - !cachefiles_ondemand_object_is_open(object)) {
>>>>>>> + !cachefiles_ondemand_object_is_open(object)) {
>>>>>>> WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>>>>>>>                xas_unlock(&xas);
>>>>>>>                ret = -EIO;
>>>>>>>                goto out;
>>>>>>>            }
>>>>>>> -        xas.xa_index = 0;
>>>>>>> +        /*
>>>>>>> +         * Cyclically find a free xas to avoid msg_id reuse that would
>>>>>>> +         * cause the daemon to successfully copen a stale msg_id.
>>>>>>> +         */
>>>>>>> +        xas.xa_index = cache->msg_id_next;
>>>>>>>            xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
>>>>>>> +        if (xas.xa_node == XAS_RESTART) {
>>>>>>> +            xas.xa_index = 0;
>>>>>>> +            xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
>>>>>>> +        }
>>>>>>>            if (xas.xa_node == XAS_RESTART)
>>>>>>>                xas_set_err(&xas, -EBUSY);
>>>>>>> +
>>>>>>>            xas_store(&xas, req);
>>>>>>> -        xas_clear_mark(&xas, XA_FREE_MARK);
>>>>>>> -        xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>>>> +        if (xas_valid(&xas)) {
>>>>>>> +            cache->msg_id_next = xas.xa_index + 1;
>>>>>> If you have a long-standing stuck request, could this counter wrap
>>>>>> around and you still end up with reuse?
>>>>> Yes, msg_id_next is declared to be of type u32 in the hope that when
>>>>> xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
>>>>> goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
>>>>> the xarry being too deep.
>>>>>
>>>>> If msg_id_next is equal to the id of a long-standing stuck request
>>>>> after the wrap-around, it is true that the reuse in the above problem
>>>>> may also occur.
>>>>>
>>>>> But I feel that a long stuck request is problematic in itself, it means
>>>>> that after we have sent 4294967295 requests, the first one has not
>>>>> been processed yet, and even if we send a million requests per
>>>>> second, this one hasn't been completed for more than an hour.
>>>>>
>>>>> We have a keep-alive process that pulls the daemon back up as
>>>>> soon as it exits, and there is a timeout mechanism for requests in
>>>>> the daemon to prevent the kernel from waiting for long periods
>>>>> of time. In other words, we should avoid the situation where
>>>>> a request is stuck for a long period of time.
>>>>>
>>>>> If you think UINT_MAX is not enough, perhaps we could raise
>>>>> the maximum value of msg_id_next to ULONG_MAX?
>>>>>> Maybe this should be using
>>>>>> ida_alloc/free instead, which would prevent that too?
>>>>>>
>>>>> The id reuse here is that the kernel has finished the open request
>>>>> req_A and freed its id_A and used it again when sending the open
>>>>> request req_B, but the daemon is still working on req_A, so the
>>>>> copen id_A succeeds but operates on req_B.
>>>>>
>>>>> The id that is being used by the kernel will not be allocated here
>>>>> so it seems that ida _alloc/free does not prevent reuse either,
>>>>> could you elaborate a bit more how this works?
>>>>>
>>>> ida_alloc and free absolutely prevent reuse while the id is in use.
>>>> That's sort of the point of those functions. Basically it uses a set of
>>>> bitmaps in an xarray to track which IDs are in use, so ida_alloc only
>>>> hands out values which are not in use. See the comments over
>>>> ida_alloc_range() in lib/idr.c.
>>>>
>>> Thank you for the explanation!
>>>
>>> The logic now provides the same guarantees as ida_alloc/free.
>>> The "reused" id, indeed, is no longer in use in the kernel, but it is still
>>> in use in the userland, so a multi-threaded daemon could be handling
>>> two different requests for the same msg_id at the same time.
>>>
>>> Previously, the logic for allocating msg_ids was to start at 0 and look
>>> for a free xas.index, so it was possible for an id to be allocated to a
>>> new request just as the id was being freed.
>>>
>>> With the change to cyclic allocation, the kernel will not use the same
>>> id again until INT_MAX requests have been sent, and during the time
>>> it takes to send requests, the daemon has enough time to process
>>> requests whose ids are still in use by the daemon, but have already
>>> been freed in the kernel.
>>
>> Again, If I understand correctly, I think the main point
>> here is
>>
>> wait_for_completion(&req_A->done)
>>
>> which could hang due to some malicious deamon.  But I think it
>> should be switched to wait_for_completion_killable() instead. *
>> It's up to users to kill the mount instance if there is a
>> malicious user daemon.
>>
>> So in that case, hung task will not be triggered anymore, and
>> you don't need to care about cyclic allocation too.
>>
>> Thanks,
>> Gao Xiang
> Hi Xiang,
> 
> The problem is not as simple as you think.
> 
> If you make it killable, it just won't trigger a hung task in
> cachefiles_ondemand_send_req(), and the process waiting for the
> resource in question will also be hung.
> 
> * When the open/read request in the mount process gets stuck,
>    the sync/drop cache will trigger a hung task panic in iterate_supers()
>    as it waits for sb->umount to be unlocked.
> * After umount, anonymous fd is not closed causing a hung task panic
>    in fscache_hash_cookie() because of waiting for cookie unhash.
> * The dentry is in a loop up state, because the read request is not being
>    processed, another process looking for the same dentry is waiting for
>    the previous lookup to finish, which triggers a hung task panic in
>    d_alloc_parallel().


As for your sb->umount, d_alloc_parallel() or even i_rwsem,
which are all currently unkillable, also see some previous
threads like:

https://lore.kernel.org/linux-fsdevel/CAJfpegu6v1fRAyLvFLOPUSAhx5aAGvPGjBWv-TDQjugqjUA_hQ@mail.gmail.com/T/#u

I don't think it's the issue of on-demand cachefiles, even
NVMe or virtio-blk or networking can be stuck in
.lookup, fill_sb or whatever.

Which can makes sb->umount, d_alloc_parallel() or even
i_rwsem unkillable.

> 
> Can all this be made killable?

I can understand your hung_task_panic concern but it
sounds like a workaround to me anyway.

Thanks,
Gao Xiang

> 
> Thanks,
> Baokun
>>
>>>
>>> Regards,
>>> Baokun
>>>>>>> + xas_clear_mark(&xas, XA_FREE_MARK);
>>>>>>> +            xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>>>> +        }
>>>>>>>            xas_unlock(&xas);
>>>>>>>        } while (xas_nomem(&xas, GFP_KERNEL));
>>>>>>>

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Baokun Li]

On 2024/5/20 22:56, Gao Xiang wrote:
> Hi Baokun,
>
> On 2024/5/20 21:24, Baokun Li wrote:
>> On 2024/5/20 20:54, Gao Xiang wrote:
>>>
>>>
>>> On 2024/5/20 20:42, Baokun Li wrote:
>>>> On 2024/5/20 18:04, Jeff Layton wrote:
>>>>> On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
>>>>>> Hi Jeff,
>>>>>>
>>>>>> Thank you very much for your review!
>>>>>>
>>>>>> On 2024/5/19 19:11, Jeff Layton wrote:
>>>>>>> On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
>>>>>>>> From: Baokun Li <libaokun1@huawei.com>
>>>>>>>>
>>>>>>>> Reusing the msg_id after a maliciously completed reopen request 
>>>>>>>> may cause
>>>>>>>> a read request to remain unprocessed and result in a hung, as 
>>>>>>>> shown below:
>>>>>>>>
>>>>>>>>          t1       |      t2       |      t3
>>>>>>>> -------------------------------------------------
>>>>>>>> cachefiles_ondemand_select_req
>>>>>>>>    cachefiles_ondemand_object_is_close(A)
>>>>>>>>    cachefiles_ondemand_set_object_reopening(A)
>>>>>>>>    queue_work(fscache_object_wq, &info->work)
>>>>>>>>                   ondemand_object_worker
>>>>>>>> cachefiles_ondemand_init_object(A)
>>>>>>>> cachefiles_ondemand_send_req(OPEN)
>>>>>>>>                       // get msg_id 6
>>>>>>>> wait_for_completion(&req_A->done)
>>>>>>>> cachefiles_ondemand_daemon_read
>>>>>>>>    // read msg_id 6 req_A
>>>>>>>>    cachefiles_ondemand_get_fd
>>>>>>>>    copy_to_user
>>>>>>>>                                   // Malicious completion msg_id 6
>>>>>>>>                                   copen 6,-1
>>>>>>>> cachefiles_ondemand_copen
>>>>>>>> complete(&req_A->done)
>>>>>>>>                                    // will not set the object 
>>>>>>>> to close
>>>>>>>>                                    // because ondemand_id && fd 
>>>>>>>> is valid.
>>>>>>>>
>>>>>>>>                   // ondemand_object_worker() is done
>>>>>>>>                   // but the object is still reopening.
>>>>>>>>
>>>>>>>>                                   // new open req_B
>>>>>>>> cachefiles_ondemand_init_object(B)
>>>>>>>> cachefiles_ondemand_send_req(OPEN)
>>>>>>>>                                    // reuse msg_id 6
>>>>>>>> process_open_req
>>>>>>>>    copen 6,A.size
>>>>>>>>    // The expected failed copen was executed successfully
>>>>>>>>
>>>>>>>> Expect copen to fail, and when it does, it closes fd, which 
>>>>>>>> sets the
>>>>>>>> object to close, and then close triggers reopen again. However, 
>>>>>>>> due to
>>>>>>>> msg_id reuse resulting in a successful copen, the anonymous fd 
>>>>>>>> is not
>>>>>>>> closed until the daemon exits. Therefore read requests waiting 
>>>>>>>> for reopen
>>>>>>>> to complete may trigger hung task.
>>>>>>>>
>>>>>>>> To avoid this issue, allocate the msg_id cyclically to avoid 
>>>>>>>> reusing the
>>>>>>>> msg_id for a very short duration of time.
>>>>>>>>
>>>>>>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when 
>>>>>>>> looking up cookie")
>>>>>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>>>>>> ---
>>>>>>>>    fs/cachefiles/internal.h |  1 +
>>>>>>>>    fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>>>>>>>>    2 files changed, 17 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>>>>>>>> index 8ecd296cc1c4..9200c00f3e98 100644
>>>>>>>> --- a/fs/cachefiles/internal.h
>>>>>>>> +++ b/fs/cachefiles/internal.h
>>>>>>>> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>>>>>>>>        unsigned long            req_id_next;
>>>>>>>>        struct xarray            ondemand_ids;    /* xarray for 
>>>>>>>> ondemand_id allocation */
>>>>>>>>        u32                ondemand_id_next;
>>>>>>>> +    u32                msg_id_next;
>>>>>>>>    };
>>>>>>>>    static inline bool cachefiles_in_ondemand_mode(struct 
>>>>>>>> cachefiles_cache *cache)
>>>>>>>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>>>>>>>> index f6440b3e7368..b10952f77472 100644
>>>>>>>> --- a/fs/cachefiles/ondemand.c
>>>>>>>> +++ b/fs/cachefiles/ondemand.c
>>>>>>>> @@ -433,20 +433,32 @@ static int 
>>>>>>>> cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>>>>>>>            smp_mb();
>>>>>>>>            if (opcode == CACHEFILES_OP_CLOSE &&
>>>>>>>> - !cachefiles_ondemand_object_is_open(object)) {
>>>>>>>> + !cachefiles_ondemand_object_is_open(object)) {
>>>>>>>> WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>>>>>>>>                xas_unlock(&xas);
>>>>>>>>                ret = -EIO;
>>>>>>>>                goto out;
>>>>>>>>            }
>>>>>>>> -        xas.xa_index = 0;
>>>>>>>> +        /*
>>>>>>>> +         * Cyclically find a free xas to avoid msg_id reuse 
>>>>>>>> that would
>>>>>>>> +         * cause the daemon to successfully copen a stale msg_id.
>>>>>>>> +         */
>>>>>>>> +        xas.xa_index = cache->msg_id_next;
>>>>>>>>            xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
>>>>>>>> +        if (xas.xa_node == XAS_RESTART) {
>>>>>>>> +            xas.xa_index = 0;
>>>>>>>> +            xas_find_marked(&xas, cache->msg_id_next - 1, 
>>>>>>>> XA_FREE_MARK);
>>>>>>>> +        }
>>>>>>>>            if (xas.xa_node == XAS_RESTART)
>>>>>>>>                xas_set_err(&xas, -EBUSY);
>>>>>>>> +
>>>>>>>>            xas_store(&xas, req);
>>>>>>>> -        xas_clear_mark(&xas, XA_FREE_MARK);
>>>>>>>> -        xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>>>>> +        if (xas_valid(&xas)) {
>>>>>>>> +            cache->msg_id_next = xas.xa_index + 1;
>>>>>>> If you have a long-standing stuck request, could this counter wrap
>>>>>>> around and you still end up with reuse?
>>>>>> Yes, msg_id_next is declared to be of type u32 in the hope that when
>>>>>> xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
>>>>>> goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
>>>>>> the xarry being too deep.
>>>>>>
>>>>>> If msg_id_next is equal to the id of a long-standing stuck request
>>>>>> after the wrap-around, it is true that the reuse in the above 
>>>>>> problem
>>>>>> may also occur.
>>>>>>
>>>>>> But I feel that a long stuck request is problematic in itself, it 
>>>>>> means
>>>>>> that after we have sent 4294967295 requests, the first one has not
>>>>>> been processed yet, and even if we send a million requests per
>>>>>> second, this one hasn't been completed for more than an hour.
>>>>>>
>>>>>> We have a keep-alive process that pulls the daemon back up as
>>>>>> soon as it exits, and there is a timeout mechanism for requests in
>>>>>> the daemon to prevent the kernel from waiting for long periods
>>>>>> of time. In other words, we should avoid the situation where
>>>>>> a request is stuck for a long period of time.
>>>>>>
>>>>>> If you think UINT_MAX is not enough, perhaps we could raise
>>>>>> the maximum value of msg_id_next to ULONG_MAX?
>>>>>>> Maybe this should be using
>>>>>>> ida_alloc/free instead, which would prevent that too?
>>>>>>>
>>>>>> The id reuse here is that the kernel has finished the open request
>>>>>> req_A and freed its id_A and used it again when sending the open
>>>>>> request req_B, but the daemon is still working on req_A, so the
>>>>>> copen id_A succeeds but operates on req_B.
>>>>>>
>>>>>> The id that is being used by the kernel will not be allocated here
>>>>>> so it seems that ida _alloc/free does not prevent reuse either,
>>>>>> could you elaborate a bit more how this works?
>>>>>>
>>>>> ida_alloc and free absolutely prevent reuse while the id is in use.
>>>>> That's sort of the point of those functions. Basically it uses a 
>>>>> set of
>>>>> bitmaps in an xarray to track which IDs are in use, so ida_alloc only
>>>>> hands out values which are not in use. See the comments over
>>>>> ida_alloc_range() in lib/idr.c.
>>>>>
>>>> Thank you for the explanation!
>>>>
>>>> The logic now provides the same guarantees as ida_alloc/free.
>>>> The "reused" id, indeed, is no longer in use in the kernel, but it 
>>>> is still
>>>> in use in the userland, so a multi-threaded daemon could be handling
>>>> two different requests for the same msg_id at the same time.
>>>>
>>>> Previously, the logic for allocating msg_ids was to start at 0 and 
>>>> look
>>>> for a free xas.index, so it was possible for an id to be allocated 
>>>> to a
>>>> new request just as the id was being freed.
>>>>
>>>> With the change to cyclic allocation, the kernel will not use the same
>>>> id again until INT_MAX requests have been sent, and during the time
>>>> it takes to send requests, the daemon has enough time to process
>>>> requests whose ids are still in use by the daemon, but have already
>>>> been freed in the kernel.
>>>
>>> Again, If I understand correctly, I think the main point
>>> here is
>>>
>>> wait_for_completion(&req_A->done)
>>>
>>> which could hang due to some malicious deamon.  But I think it
>>> should be switched to wait_for_completion_killable() instead. *
>>> It's up to users to kill the mount instance if there is a
>>> malicious user daemon.
>>>
>>> So in that case, hung task will not be triggered anymore, and
>>> you don't need to care about cyclic allocation too.
>>>
>>> Thanks,
>>> Gao Xiang
>> Hi Xiang,
>>
>> The problem is not as simple as you think.
>>
>> If you make it killable, it just won't trigger a hung task in
>> cachefiles_ondemand_send_req(), and the process waiting for the
>> resource in question will also be hung.
>>
>> * When the open/read request in the mount process gets stuck,
>>    the sync/drop cache will trigger a hung task panic in 
>> iterate_supers()
>>    as it waits for sb->umount to be unlocked.
>> * After umount, anonymous fd is not closed causing a hung task panic
>>    in fscache_hash_cookie() because of waiting for cookie unhash.
>> * The dentry is in a loop up state, because the read request is not 
>> being
>>    processed, another process looking for the same dentry is waiting for
>>    the previous lookup to finish, which triggers a hung task panic in
>>    d_alloc_parallel().
>
>
> As for your sb->umount, d_alloc_parallel() or even i_rwsem,
> which are all currently unkillable, also see some previous
> threads like:
>
> https://lore.kernel.org/linux-fsdevel/CAJfpegu6v1fRAyLvFLOPUSAhx5aAGvPGjBWv-TDQjugqjUA_hQ@mail.gmail.com/T/#u 
>
>
> I don't think it's the issue of on-demand cachefiles, even
> NVMe or virtio-blk or networking can be stuck in
> .lookup, fill_sb or whatever.
>
> Which can makes sb->umount, d_alloc_parallel() or even
> i_rwsem unkillable.
>
Everyone and every company has different requirements for quality,
and if you don't think these hung_task_panic are problems, I respect
your opinion.

But the company I work for has much higher requirements for quality,
and it's not acceptable to leave these issues that have been tested out
unresolved.
>>
>> Can all this be made killable?
>
> I can understand your hung_task_panic concern but it
> sounds like a workaround to me anyway.
>
> Thanks,
> Gao Xiang
>
 From the current perspective, cyclic allocation is a valid solution to
the current msg_id collision problem, and it also makes it fairer to
copy out requests than it was before.

Thanks!

-- 
With Best Regards,
Baokun Li

Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse

[Gao Xiang]

On 2024/5/21 10:36, Baokun Li wrote:
> On 2024/5/20 22:56, Gao Xiang wrote:
>> Hi Baokun,
>>
>> On 2024/5/20 21:24, Baokun Li wrote:
>>> On 2024/5/20 20:54, Gao Xiang wrote:
>>>>
>>>>
>>>> On 2024/5/20 20:42, Baokun Li wrote:
>>>>> On 2024/5/20 18:04, Jeff Layton wrote:
>>>>>> On Mon, 2024-05-20 at 12:06 +0800, Baokun Li wrote:
>>>>>>> Hi Jeff,
>>>>>>>
>>>>>>> Thank you very much for your review!
>>>>>>>
>>>>>>> On 2024/5/19 19:11, Jeff Layton wrote:
>>>>>>>> On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
>>>>>>>>> From: Baokun Li <libaokun1@huawei.com>
>>>>>>>>>
>>>>>>>>> Reusing the msg_id after a maliciously completed reopen request may cause
>>>>>>>>> a read request to remain unprocessed and result in a hung, as shown below:
>>>>>>>>>
>>>>>>>>>          t1       |      t2       |      t3
>>>>>>>>> -------------------------------------------------
>>>>>>>>> cachefiles_ondemand_select_req
>>>>>>>>>    cachefiles_ondemand_object_is_close(A)
>>>>>>>>>    cachefiles_ondemand_set_object_reopening(A)
>>>>>>>>>    queue_work(fscache_object_wq, &info->work)
>>>>>>>>>                   ondemand_object_worker
>>>>>>>>> cachefiles_ondemand_init_object(A)
>>>>>>>>> cachefiles_ondemand_send_req(OPEN)
>>>>>>>>>                       // get msg_id 6
>>>>>>>>> wait_for_completion(&req_A->done)
>>>>>>>>> cachefiles_ondemand_daemon_read
>>>>>>>>>    // read msg_id 6 req_A
>>>>>>>>>    cachefiles_ondemand_get_fd
>>>>>>>>>    copy_to_user
>>>>>>>>>                                   // Malicious completion msg_id 6
>>>>>>>>>                                   copen 6,-1
>>>>>>>>> cachefiles_ondemand_copen
>>>>>>>>> complete(&req_A->done)
>>>>>>>>>                                    // will not set the object to close
>>>>>>>>>                                    // because ondemand_id && fd is valid.
>>>>>>>>>
>>>>>>>>>                   // ondemand_object_worker() is done
>>>>>>>>>                   // but the object is still reopening.
>>>>>>>>>
>>>>>>>>>                                   // new open req_B
>>>>>>>>> cachefiles_ondemand_init_object(B)
>>>>>>>>> cachefiles_ondemand_send_req(OPEN)
>>>>>>>>>                                    // reuse msg_id 6
>>>>>>>>> process_open_req
>>>>>>>>>    copen 6,A.size
>>>>>>>>>    // The expected failed copen was executed successfully
>>>>>>>>>
>>>>>>>>> Expect copen to fail, and when it does, it closes fd, which sets the
>>>>>>>>> object to close, and then close triggers reopen again. However, due to
>>>>>>>>> msg_id reuse resulting in a successful copen, the anonymous fd is not
>>>>>>>>> closed until the daemon exits. Therefore read requests waiting for reopen
>>>>>>>>> to complete may trigger hung task.
>>>>>>>>>
>>>>>>>>> To avoid this issue, allocate the msg_id cyclically to avoid reusing the
>>>>>>>>> msg_id for a very short duration of time.
>>>>>>>>>
>>>>>>>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
>>>>>>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>>>>>>> ---
>>>>>>>>>    fs/cachefiles/internal.h |  1 +
>>>>>>>>>    fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
>>>>>>>>>    2 files changed, 17 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>>>>>>>>> index 8ecd296cc1c4..9200c00f3e98 100644
>>>>>>>>> --- a/fs/cachefiles/internal.h
>>>>>>>>> +++ b/fs/cachefiles/internal.h
>>>>>>>>> @@ -128,6 +128,7 @@ struct cachefiles_cache {
>>>>>>>>>        unsigned long            req_id_next;
>>>>>>>>>        struct xarray            ondemand_ids;    /* xarray for ondemand_id allocation */
>>>>>>>>>        u32                ondemand_id_next;
>>>>>>>>> +    u32                msg_id_next;
>>>>>>>>>    };
>>>>>>>>>    static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
>>>>>>>>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>>>>>>>>> index f6440b3e7368..b10952f77472 100644
>>>>>>>>> --- a/fs/cachefiles/ondemand.c
>>>>>>>>> +++ b/fs/cachefiles/ondemand.c
>>>>>>>>> @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>>>>>>>>            smp_mb();
>>>>>>>>>            if (opcode == CACHEFILES_OP_CLOSE &&
>>>>>>>>> - !cachefiles_ondemand_object_is_open(object)) {
>>>>>>>>> + !cachefiles_ondemand_object_is_open(object)) {
>>>>>>>>> WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
>>>>>>>>>                xas_unlock(&xas);
>>>>>>>>>                ret = -EIO;
>>>>>>>>>                goto out;
>>>>>>>>>            }
>>>>>>>>> -        xas.xa_index = 0;
>>>>>>>>> +        /*
>>>>>>>>> +         * Cyclically find a free xas to avoid msg_id reuse that would
>>>>>>>>> +         * cause the daemon to successfully copen a stale msg_id.
>>>>>>>>> +         */
>>>>>>>>> +        xas.xa_index = cache->msg_id_next;
>>>>>>>>>            xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
>>>>>>>>> +        if (xas.xa_node == XAS_RESTART) {
>>>>>>>>> +            xas.xa_index = 0;
>>>>>>>>> +            xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
>>>>>>>>> +        }
>>>>>>>>>            if (xas.xa_node == XAS_RESTART)
>>>>>>>>>                xas_set_err(&xas, -EBUSY);
>>>>>>>>> +
>>>>>>>>>            xas_store(&xas, req);
>>>>>>>>> -        xas_clear_mark(&xas, XA_FREE_MARK);
>>>>>>>>> -        xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>>>>>>>>> +        if (xas_valid(&xas)) {
>>>>>>>>> +            cache->msg_id_next = xas.xa_index + 1;
>>>>>>>> If you have a long-standing stuck request, could this counter wrap
>>>>>>>> around and you still end up with reuse?
>>>>>>> Yes, msg_id_next is declared to be of type u32 in the hope that when
>>>>>>> xa_index == UINT_MAX, a wrap around occurs so that msg_id_next
>>>>>>> goes to zero. Limiting xa_index to no more than UINT_MAX is to avoid
>>>>>>> the xarry being too deep.
>>>>>>>
>>>>>>> If msg_id_next is equal to the id of a long-standing stuck request
>>>>>>> after the wrap-around, it is true that the reuse in the above problem
>>>>>>> may also occur.
>>>>>>>
>>>>>>> But I feel that a long stuck request is problematic in itself, it means
>>>>>>> that after we have sent 4294967295 requests, the first one has not
>>>>>>> been processed yet, and even if we send a million requests per
>>>>>>> second, this one hasn't been completed for more than an hour.
>>>>>>>
>>>>>>> We have a keep-alive process that pulls the daemon back up as
>>>>>>> soon as it exits, and there is a timeout mechanism for requests in
>>>>>>> the daemon to prevent the kernel from waiting for long periods
>>>>>>> of time. In other words, we should avoid the situation where
>>>>>>> a request is stuck for a long period of time.
>>>>>>>
>>>>>>> If you think UINT_MAX is not enough, perhaps we could raise
>>>>>>> the maximum value of msg_id_next to ULONG_MAX?
>>>>>>>> Maybe this should be using
>>>>>>>> ida_alloc/free instead, which would prevent that too?
>>>>>>>>
>>>>>>> The id reuse here is that the kernel has finished the open request
>>>>>>> req_A and freed its id_A and used it again when sending the open
>>>>>>> request req_B, but the daemon is still working on req_A, so the
>>>>>>> copen id_A succeeds but operates on req_B.
>>>>>>>
>>>>>>> The id that is being used by the kernel will not be allocated here
>>>>>>> so it seems that ida _alloc/free does not prevent reuse either,
>>>>>>> could you elaborate a bit more how this works?
>>>>>>>
>>>>>> ida_alloc and free absolutely prevent reuse while the id is in use.
>>>>>> That's sort of the point of those functions. Basically it uses a set of
>>>>>> bitmaps in an xarray to track which IDs are in use, so ida_alloc only
>>>>>> hands out values which are not in use. See the comments over
>>>>>> ida_alloc_range() in lib/idr.c.
>>>>>>
>>>>> Thank you for the explanation!
>>>>>
>>>>> The logic now provides the same guarantees as ida_alloc/free.
>>>>> The "reused" id, indeed, is no longer in use in the kernel, but it is still
>>>>> in use in the userland, so a multi-threaded daemon could be handling
>>>>> two different requests for the same msg_id at the same time.
>>>>>
>>>>> Previously, the logic for allocating msg_ids was to start at 0 and look
>>>>> for a free xas.index, so it was possible for an id to be allocated to a
>>>>> new request just as the id was being freed.
>>>>>
>>>>> With the change to cyclic allocation, the kernel will not use the same
>>>>> id again until INT_MAX requests have been sent, and during the time
>>>>> it takes to send requests, the daemon has enough time to process
>>>>> requests whose ids are still in use by the daemon, but have already
>>>>> been freed in the kernel.
>>>>
>>>> Again, If I understand correctly, I think the main point
>>>> here is
>>>>
>>>> wait_for_completion(&req_A->done)
>>>>
>>>> which could hang due to some malicious deamon.  But I think it
>>>> should be switched to wait_for_completion_killable() instead. *
>>>> It's up to users to kill the mount instance if there is a
>>>> malicious user daemon.
>>>>
>>>> So in that case, hung task will not be triggered anymore, and
>>>> you don't need to care about cyclic allocation too.
>>>>
>>>> Thanks,
>>>> Gao Xiang
>>> Hi Xiang,
>>>
>>> The problem is not as simple as you think.
>>>
>>> If you make it killable, it just won't trigger a hung task in
>>> cachefiles_ondemand_send_req(), and the process waiting for the
>>> resource in question will also be hung.
>>>
>>> * When the open/read request in the mount process gets stuck,
>>>    the sync/drop cache will trigger a hung task panic in iterate_supers()
>>>    as it waits for sb->umount to be unlocked.
>>> * After umount, anonymous fd is not closed causing a hung task panic
>>>    in fscache_hash_cookie() because of waiting for cookie unhash.
>>> * The dentry is in a loop up state, because the read request is not being
>>>    processed, another process looking for the same dentry is waiting for
>>>    the previous lookup to finish, which triggers a hung task panic in
>>>    d_alloc_parallel().
>>
>>
>> As for your sb->umount, d_alloc_parallel() or even i_rwsem,
>> which are all currently unkillable, also see some previous
>> threads like:
>>
>> https://lore.kernel.org/linux-fsdevel/CAJfpegu6v1fRAyLvFLOPUSAhx5aAGvPGjBWv-TDQjugqjUA_hQ@mail.gmail.com/T/#u
>>
>> I don't think it's the issue of on-demand cachefiles, even
>> NVMe or virtio-blk or networking can be stuck in
>> .lookup, fill_sb or whatever.
>>
>> Which can makes sb->umount, d_alloc_parallel() or even
>> i_rwsem unkillable.
>>
> Everyone and every company has different requirements for quality,
> and if you don't think these hung_task_panic are problems, I respect
> your opinion.
> 
> But the company I work for has much higher requirements for quality,
> and it's not acceptable to leave these issues that have been tested out
> unresolved.
>>>
>>> Can all this be made killable?
>>
>> I can understand your hung_task_panic concern but it
>> sounds like a workaround to me anyway.
>>
>> Thanks,
>> Gao Xiang
>>
>  From the current perspective, cyclic allocation is a valid solution to
> the current msg_id collision problem, and it also makes it fairer to
> copy out requests than it was before.

Okay, for this patch, I agree it's better than none and it can
indeed cause fairer requests, so

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> 
> Thanks!
>