2022-10-14 03:28:06

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V2 0/5] Introduce daemon failover mechanism to recover from crashing

Changes since V1:
1. Extract cachefiles_ondemand_select_req() from cachefiles_ondemand_daemon_read()
to make the code more readable.
2. Fix a UAF bug reported by JeffleXu.
3. Modify some code comments.

[Background]
============
In ondemand read mode, if user daemon closes anonymous fd(e.g. daemon
crashes), subsequent read and inflight requests based on these fd will
return -EIO.
Even if above mentioned case is tolerable for some individual users, but
when it happenens in real cloud service production environment, such IO
errors will be passed to cloud service users and impact its working jobs.
It's terrible for cloud service stability.

[Design]
========
This patchset introduce three states for ondemand object:
CLOSE: Object which just be allocated or closed by user daemon.
OPEN: Object which related OPEN request has been processed correctly.
REOPENING: Object which has been closed, and is drived to open by a read
request.

[Flow Path]
===========
[Daemon Crash]
0. Daemon use UDS send/receive fd to keep and pass the fd reference of
"/dev/cachefiles".
1. User daemon crashes -> restart and recover dev fd's reference.
2. User daemon write "restore" to device.
2.1 Reset the object's state from CLOSE to OPENING.
2.2 Init a work which reinit the object and add it to wq. (daemon can
get rid of kernel space and handle that open request).
3. The user of upper filesystem won't notice that the daemon ever crashed
since the inflight IO is restored and handled correctly.

[Daemon Close fd]
1. User daemon closes an anonymous fd.
2. User daemon reads a READ request which the associated anonymous fd was
closed and init a work which re-open the object.
3. User daemon handles above open request normally.
4. The user of upper filesystem won't notice that the daemon ever closed
any fd since the closed object is re-opened and related request was
handled correctly.

[Test]
======
There is a testcase for above mentioned scenario.
A user process read the file by fscache ondemand reading.
At the same time, we kill the daemon constantly.
The expected result is that the file read by user is consistent with
original, and the user doesn't notice that daemon has ever been killed.

https://github.com/userzj/demand-read-cachefilesd/commits/failover-test

[GitWeb]
========
https://github.com/userzj/linux/tree/fscache-failover-v3

Jia Zhu (5):
cachefiles: introduce object ondemand state
cachefiles: extract ondemand info field from cachefiles_object
cachefiles: resend an open request if the read request's object is
closed
cachefiles: narrow the scope of triggering EPOLLIN events in ondemand
mode
cachefiles: add restore command to recover inflight ondemand read
requests

fs/cachefiles/daemon.c | 14 +++-
fs/cachefiles/interface.c | 6 ++
fs/cachefiles/internal.h | 58 +++++++++++++-
fs/cachefiles/ondemand.c | 156 ++++++++++++++++++++++++++++----------
4 files changed, 188 insertions(+), 46 deletions(-)

--
2.20.1


2022-10-14 03:28:54

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V2 5/5] cachefiles: add restore command to recover inflight ondemand read requests

Previously, in ondemand read scenario, if the anonymous fd was closed by
user daemon, inflight and subsequent read requests would return EIO.
As long as the device connection is not released, user daemon can hold
and restore inflight requests by setting the request flag to
CACHEFILES_REQ_NEW.

Suggested-by: Gao Xiang <[email protected]>
Signed-off-by: Jia Zhu <[email protected]>
Signed-off-by: Xin Yin <[email protected]>
---
fs/cachefiles/daemon.c | 1 +
fs/cachefiles/internal.h | 3 +++
fs/cachefiles/ondemand.c | 23 +++++++++++++++++++++++
3 files changed, 27 insertions(+)

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index c74bd1f4ecf5..014369266cb2 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -77,6 +77,7 @@ static const struct cachefiles_daemon_cmd cachefiles_daemon_cmds[] = {
{ "tag", cachefiles_daemon_tag },
#ifdef CONFIG_CACHEFILES_ONDEMAND
{ "copen", cachefiles_ondemand_copen },
+ { "restore", cachefiles_ondemand_restore },
#endif
{ "", NULL }
};
diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 98d6cf58db11..a3cacba57def 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -302,6 +302,9 @@ extern ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
extern int cachefiles_ondemand_copen(struct cachefiles_cache *cache,
char *args);

+extern int cachefiles_ondemand_restore(struct cachefiles_cache *cache,
+ char *args);
+
extern int cachefiles_ondemand_init_object(struct cachefiles_object *object);
extern void cachefiles_ondemand_clean_object(struct cachefiles_object *object);

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index c9eea89befec..08677c9d0004 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -182,6 +182,29 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
return ret;
}

+int cachefiles_ondemand_restore(struct cachefiles_cache *cache, char *args)
+{
+ struct cachefiles_req *req;
+
+ XA_STATE(xas, &cache->reqs, 0);
+
+ if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
+ return -EOPNOTSUPP;
+
+ /*
+ * Reset the requests to CACHEFILES_REQ_NEW state, so that the
+ * requests have been processed halfway before the crash of the
+ * user daemon could be reprocessed after the recovery.
+ */
+ xas_lock(&xas);
+ xas_for_each(&xas, req, ULONG_MAX)
+ xas_set_mark(&xas, CACHEFILES_REQ_NEW);
+ xas_unlock(&xas);
+
+ wake_up_all(&cache->daemon_pollwq);
+ return 0;
+}
+
static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
{
struct cachefiles_object *object;
--
2.20.1

2022-10-14 03:48:42

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V2 1/5] cachefiles: introduce object ondemand state

Previously, @ondemand_id field was used not only to identify ondemand
state of the object, but also to represent the index of the xarray.
This commit introduces @state field to decouple the role of @ondemand_id
and adds helpers to access it.

Signed-off-by: Jia Zhu <[email protected]>
Reviewed-by: Xin Yin <[email protected]>
---
fs/cachefiles/internal.h | 22 ++++++++++++++++++++++
fs/cachefiles/ondemand.c | 21 +++++++++------------
2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 2ad58c465208..39895bbd149a 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -17,6 +17,7 @@
#include <linux/security.h>
#include <linux/xarray.h>
#include <linux/cachefiles.h>
+#include <linux/atomic.h>

#define CACHEFILES_DIO_BLOCK_SIZE 4096

@@ -44,6 +45,11 @@ struct cachefiles_volume {
struct dentry *fanout[256]; /* Fanout subdirs */
};

+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 */
+};
+
/*
* Backing file state.
*/
@@ -62,6 +68,7 @@ struct cachefiles_object {
#define CACHEFILES_OBJECT_USING_TMPFILE 0 /* Have an unlinked tmpfile */
#ifdef CONFIG_CACHEFILES_ONDEMAND
int ondemand_id;
+ enum cachefiles_object_state state;
#endif
};

@@ -296,6 +303,21 @@ extern void cachefiles_ondemand_clean_object(struct cachefiles_object *object);
extern int cachefiles_ondemand_read(struct cachefiles_object *object,
loff_t pos, size_t len);

+#define CACHEFILES_OBJECT_STATE_FUNCS(_state) \
+static inline bool \
+cachefiles_ondemand_object_is_##_state(const struct cachefiles_object *object) \
+{ \
+ return object->state == CACHEFILES_ONDEMAND_OBJSTATE_##_state; \
+} \
+ \
+static inline void \
+cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
+{ \
+ object->state = CACHEFILES_ONDEMAND_OBJSTATE_##_state; \
+}
+
+CACHEFILES_OBJECT_STATE_FUNCS(open);
+CACHEFILES_OBJECT_STATE_FUNCS(close);
#else
static inline ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
char __user *_buffer, size_t buflen)
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 0254ed39f68c..90456b8a4b3e 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -15,6 +15,7 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,

xa_lock(&cache->reqs);
object->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
+ cachefiles_ondemand_set_object_close(object);

/*
* Flush all pending READ requests since their completion depends on
@@ -176,6 +177,8 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
set_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);
trace_cachefiles_ondemand_copen(req->object, id, size);

+ cachefiles_ondemand_set_object_open(req->object);
+
out:
complete(&req->done);
return ret;
@@ -363,7 +366,8 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
/* coupled with the barrier in cachefiles_flush_reqs() */
smp_mb();

- if (opcode != CACHEFILES_OP_OPEN && object->ondemand_id <= 0) {
+ if (opcode != CACHEFILES_OP_OPEN &&
+ !cachefiles_ondemand_object_is_open(object)) {
WARN_ON_ONCE(object->ondemand_id == 0);
xas_unlock(&xas);
ret = -EIO;
@@ -430,18 +434,11 @@ static int cachefiles_ondemand_init_close_req(struct cachefiles_req *req,
void *private)
{
struct cachefiles_object *object = req->object;
- int object_id = object->ondemand_id;

- /*
- * It's possible that object id is still 0 if the cookie looking up
- * phase failed before OPEN request has ever been sent. Also avoid
- * sending CLOSE request for CACHEFILES_ONDEMAND_ID_CLOSED, which means
- * anon_fd has already been closed.
- */
- if (object_id <= 0)
+ if (!cachefiles_ondemand_object_is_open(object))
return -ENOENT;

- req->msg.object_id = object_id;
+ req->msg.object_id = object->ondemand_id;
trace_cachefiles_ondemand_close(object, &req->msg);
return 0;
}
@@ -460,7 +457,7 @@ static int cachefiles_ondemand_init_read_req(struct cachefiles_req *req,
int object_id = object->ondemand_id;

/* Stop enqueuing requests when daemon has closed anon_fd. */
- if (object_id <= 0) {
+ if (!cachefiles_ondemand_object_is_open(object)) {
WARN_ON_ONCE(object_id == 0);
pr_info_once("READ: anonymous fd closed prematurely.\n");
return -EIO;
@@ -485,7 +482,7 @@ int cachefiles_ondemand_init_object(struct cachefiles_object *object)
* creating a new tmpfile as the cache file. Reuse the previously
* allocated object ID if any.
*/
- if (object->ondemand_id > 0)
+ if (cachefiles_ondemand_object_is_open(object))
return 0;

volume_key_size = volume->key[0] + 1;
--
2.20.1

2022-10-14 03:52:30

by Jia Zhu

[permalink] [raw]
Subject: [PATCH V2 4/5] cachefiles: narrow the scope of triggering EPOLLIN events in ondemand mode

Don't trigger EPOLLIN when there are only reopening read requests in
xarray.

Suggested-by: Xin Yin <[email protected]>
Signed-off-by: Jia Zhu <[email protected]>
---
fs/cachefiles/daemon.c | 13 +++++++++++--
fs/cachefiles/internal.h | 12 ++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index aa4efcabb5e3..c74bd1f4ecf5 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -355,14 +355,23 @@ static __poll_t cachefiles_daemon_poll(struct file *file,
struct poll_table_struct *poll)
{
struct cachefiles_cache *cache = file->private_data;
+ struct xarray *xa = &cache->reqs;
+ struct cachefiles_req *req;
+ unsigned long index;
__poll_t mask;

poll_wait(file, &cache->daemon_pollwq, poll);
mask = 0;

if (cachefiles_in_ondemand_mode(cache)) {
- if (!xa_empty(&cache->reqs))
- mask |= EPOLLIN;
+ if (!xa_empty(xa)) {
+ xa_for_each_marked(xa, index, req, CACHEFILES_REQ_NEW) {
+ if (!cachefiles_ondemand_is_reopening_read(req)) {
+ mask |= EPOLLIN;
+ break;
+ }
+ }
+ }
} else {
if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))
mask |= EPOLLIN;
diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 21ef5007f488..98d6cf58db11 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -327,6 +327,13 @@ cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
CACHEFILES_OBJECT_STATE_FUNCS(open);
CACHEFILES_OBJECT_STATE_FUNCS(close);
CACHEFILES_OBJECT_STATE_FUNCS(reopening);
+
+static inline bool cachefiles_ondemand_is_reopening_read(struct cachefiles_req *req)
+{
+ return cachefiles_ondemand_object_is_reopening(req->object) &&
+ req->msg.opcode == CACHEFILES_OP_READ;
+}
+
#else
static inline ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
char __user *_buffer, size_t buflen)
@@ -354,6 +361,11 @@ static inline int cachefiles_ondemand_init_obj_info(struct cachefiles_object *ob
{
return 0;
}
+
+static inline bool cachefiles_ondemand_is_reopening_read(struct cachefiles_req *req)
+{
+ return false;
+}
#endif

/*
--
2.20.1

2022-10-14 06:47:30

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH V2 4/5] cachefiles: narrow the scope of triggering EPOLLIN events in ondemand mode



On 10/14/22 11:07 AM, Jia Zhu wrote:
> Don't trigger EPOLLIN when there are only reopening read requests in
> xarray.
>
> Suggested-by: Xin Yin <[email protected]>
> Signed-off-by: Jia Zhu <[email protected]>

LGTM.

Reviewed-by: Jingbo Xu <[email protected]>

--
Thanks,
Jingbo

2022-10-14 07:13:07

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH V2 1/5] cachefiles: introduce object ondemand state



On 10/14/22 11:07 AM, Jia Zhu wrote:
> Previously, @ondemand_id field was used not only to identify ondemand
> state of the object, but also to represent the index of the xarray.
> This commit introduces @state field to decouple the role of @ondemand_id
> and adds helpers to access it.
>
> Signed-off-by: Jia Zhu <[email protected]>
> Reviewed-by: Xin Yin <[email protected]>
> ---
> fs/cachefiles/internal.h | 22 ++++++++++++++++++++++
> fs/cachefiles/ondemand.c | 21 +++++++++------------
> 2 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index 2ad58c465208..39895bbd149a 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -17,6 +17,7 @@
> #include <linux/security.h>
> #include <linux/xarray.h>
> #include <linux/cachefiles.h>
> +#include <linux/atomic.h>
^
This is not needed any more.

Otherwise LGTM.

Reviewed-by: Jingbo Xu <[email protected]>


--
Thanks,
Jingbo

2022-10-14 07:16:06

by Jingbo Xu

[permalink] [raw]
Subject: Re: [PATCH V2 5/5] cachefiles: add restore command to recover inflight ondemand read requests



On 10/14/22 11:07 AM, Jia Zhu wrote:
> Previously, in ondemand read scenario, if the anonymous fd was closed by
> user daemon, inflight and subsequent read requests would return EIO.
> As long as the device connection is not released, user daemon can hold
> and restore inflight requests by setting the request flag to
> CACHEFILES_REQ_NEW.
>
> Suggested-by: Gao Xiang <[email protected]>
> Signed-off-by: Jia Zhu <[email protected]>
> Signed-off-by: Xin Yin <[email protected]>

LGTM.

Reviewed-by: Jingbo Xu <[email protected]>

--
Thanks,
Jingbo