| Message ID | 20240515084601.3240503-9-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cachefiles: some bugfixes and cleanups for ondemand requests | expand |
On 5/15/24 4:45 PM, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > Now every time the daemon reads an open request, it gets a new anonymous fd > and ondemand_id. With the introduction of "restore", it is possible to read > the same open request more than once, and therefore an object can have more > than one anonymous fd. > > If the anonymous fd is not unique, the following concurrencies will result > in an fd leak: > > t1 | t2 | t3 > ------------------------------------------------------------ > cachefiles_ondemand_init_object > cachefiles_ondemand_send_req > REQ_A = kzalloc(sizeof(*req) + data_len) > wait_for_completion(&REQ_A->done) > cachefiles_daemon_read > cachefiles_ondemand_daemon_read > REQ_A = cachefiles_ondemand_select_req > cachefiles_ondemand_get_fd > load->fd = fd0 > ondemand_id = object_id0 > ------ restore ------ > cachefiles_ondemand_restore > // restore REQ_A > cachefiles_daemon_read > cachefiles_ondemand_daemon_read > REQ_A = cachefiles_ondemand_select_req > cachefiles_ondemand_get_fd > load->fd = fd1 > ondemand_id = object_id1 > process_open_req(REQ_A) > write(devfd, ("copen %u,%llu", msg->msg_id, size)) > cachefiles_ondemand_copen > xa_erase(&cache->reqs, id) > complete(&REQ_A->done) > kfree(REQ_A) > process_open_req(REQ_A) > // copen fails due to no req > // daemon close(fd1) > cachefiles_ondemand_fd_release > // set object closed > -- umount -- > cachefiles_withdraw_cookie > cachefiles_ondemand_clean_object > cachefiles_ondemand_init_close_req > if (!cachefiles_ondemand_object_is_open(object)) > return -ENOENT; > // The fd0 is not closed until the daemon exits. > > However, the anonymous fd holds the reference count of the object and the > object holds the reference count of the cookie. So even though the cookie > has been relinquished, it will not be unhashed and freed until the daemon > exits. > > In fscache_hash_cookie(), when the same cookie is found in the hash list, > if the cookie is set with the FSCACHE_COOKIE_RELINQUISHED bit, then the new > cookie waits for the old cookie to be unhashed, while the old cookie is > waiting for the leaked fd to be closed, if the daemon does not exit in time > it will trigger a hung task. > > To avoid this, allocate a new anonymous fd only if no anonymous fd has > been allocated (ondemand_id == 0) or if the previously allocated anonymous > fd has been closed (ondemand_id == -1). Moreover, returns an error if > ondemand_id is valid, letting the daemon know that the current userland > restore logic is abnormal and needs to be checked. > > Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie") > Signed-off-by: Baokun Li <libaokun1@huawei.com> The LOCs of this fix is quite under control. But still it seems that the worst consequence is that the (potential) malicious daemon gets hung. No more effect to the system or other processes. Or does a non-malicious daemon have any chance having the same issue? > --- > fs/cachefiles/ondemand.c | 34 ++++++++++++++++++++++++++++------ > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c > index d04ddc6576e3..d2d4e27fca6f 100644 > --- a/fs/cachefiles/ondemand.c > +++ b/fs/cachefiles/ondemand.c > @@ -14,11 +14,18 @@ static int cachefiles_ondemand_fd_release(struct inode *inode, > struct file *file) > { > struct cachefiles_object *object = file->private_data; > - struct cachefiles_cache *cache = object->volume->cache; > - struct cachefiles_ondemand_info *info = object->ondemand; > + struct cachefiles_cache *cache; > + struct cachefiles_ondemand_info *info; > int object_id; > struct cachefiles_req *req; > - XA_STATE(xas, &cache->reqs, 0); > + XA_STATE(xas, NULL, 0); > + > + if (!object) > + return 0; > + > + info = object->ondemand; > + cache = object->volume->cache; > + xas.xa = &cache->reqs; > > xa_lock(&cache->reqs); > spin_lock(&info->lock); > @@ -288,22 +295,39 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req) > goto err_put_fd; > } > > + spin_lock(&object->ondemand->lock); > + if (object->ondemand->ondemand_id > 0) { > + spin_unlock(&object->ondemand->lock); > + /* Pair with check in cachefiles_ondemand_fd_release(). */ > + file->private_data = NULL; > + ret = -EEXIST; > + goto err_put_file; > + } > + > file->f_mode |= FMODE_PWRITE | FMODE_LSEEK; > fd_install(fd, file); > > load = (void *)req->msg.data; > load->fd = fd; > object->ondemand->ondemand_id = object_id; > + spin_unlock(&object->ondemand->lock); > > cachefiles_get_unbind_pincount(cache); > trace_cachefiles_ondemand_open(object, &req->msg, load); > return 0; > > +err_put_file: > + fput(file); > err_put_fd: > put_unused_fd(fd); > err_free_id: > xa_erase(&cache->ondemand_ids, object_id); > err: > + spin_lock(&object->ondemand->lock); > + /* Avoid marking an opened object as closed. */ > + if (object->ondemand->ondemand_id <= 0) > + cachefiles_ondemand_set_object_close(object); > + spin_unlock(&object->ondemand->lock); > cachefiles_put_object(object, cachefiles_obj_put_ondemand_fd); > return ret; > } > @@ -386,10 +410,8 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache, > > if (msg->opcode == CACHEFILES_OP_OPEN) { > ret = cachefiles_ondemand_get_fd(req); > - if (ret) { > - cachefiles_ondemand_set_object_close(req->object); > + if (ret) > goto out; > - } > } > > msg->msg_id = xas.xa_index;
On 2024/5/20 16:43, Jingbo Xu wrote: > > On 5/15/24 4:45 PM, libaokun@huaweicloud.com wrote: >> From: Baokun Li <libaokun1@huawei.com> >> >> Now every time the daemon reads an open request, it gets a new anonymous fd >> and ondemand_id. With the introduction of "restore", it is possible to read >> the same open request more than once, and therefore an object can have more >> than one anonymous fd. >> >> If the anonymous fd is not unique, the following concurrencies will result >> in an fd leak: >> >> t1 | t2 | t3 >> ------------------------------------------------------------ >> cachefiles_ondemand_init_object >> cachefiles_ondemand_send_req >> REQ_A = kzalloc(sizeof(*req) + data_len) >> wait_for_completion(&REQ_A->done) >> cachefiles_daemon_read >> cachefiles_ondemand_daemon_read >> REQ_A = cachefiles_ondemand_select_req >> cachefiles_ondemand_get_fd >> load->fd = fd0 >> ondemand_id = object_id0 >> ------ restore ------ >> cachefiles_ondemand_restore >> // restore REQ_A >> cachefiles_daemon_read >> cachefiles_ondemand_daemon_read >> REQ_A = cachefiles_ondemand_select_req >> cachefiles_ondemand_get_fd >> load->fd = fd1 >> ondemand_id = object_id1 >> process_open_req(REQ_A) >> write(devfd, ("copen %u,%llu", msg->msg_id, size)) >> cachefiles_ondemand_copen >> xa_erase(&cache->reqs, id) >> complete(&REQ_A->done) >> kfree(REQ_A) >> process_open_req(REQ_A) >> // copen fails due to no req >> // daemon close(fd1) >> cachefiles_ondemand_fd_release >> // set object closed >> -- umount -- >> cachefiles_withdraw_cookie >> cachefiles_ondemand_clean_object >> cachefiles_ondemand_init_close_req >> if (!cachefiles_ondemand_object_is_open(object)) >> return -ENOENT; >> // The fd0 is not closed until the daemon exits. >> >> However, the anonymous fd holds the reference count of the object and the >> object holds the reference count of the cookie. So even though the cookie >> has been relinquished, it will not be unhashed and freed until the daemon >> exits. >> >> In fscache_hash_cookie(), when the same cookie is found in the hash list, >> if the cookie is set with the FSCACHE_COOKIE_RELINQUISHED bit, then the new >> cookie waits for the old cookie to be unhashed, while the old cookie is >> waiting for the leaked fd to be closed, if the daemon does not exit in time >> it will trigger a hung task. >> >> To avoid this, allocate a new anonymous fd only if no anonymous fd has >> been allocated (ondemand_id == 0) or if the previously allocated anonymous >> fd has been closed (ondemand_id == -1). Moreover, returns an error if >> ondemand_id is valid, letting the daemon know that the current userland >> restore logic is abnormal and needs to be checked. >> >> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie") >> Signed-off-by: Baokun Li <libaokun1@huawei.com> > The LOCs of this fix is quite under control. But still it seems that > the worst consequence is that the (potential) malicious daemon gets > hung. No more effect to the system or other processes. Or does a > non-malicious daemon have any chance having the same issue? If we enable hung_task_panic, it may cause panic to crash the server. >> --- >> fs/cachefiles/ondemand.c | 34 ++++++++++++++++++++++++++++------ >> 1 file changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c >> index d04ddc6576e3..d2d4e27fca6f 100644 >> --- a/fs/cachefiles/ondemand.c >> +++ b/fs/cachefiles/ondemand.c >> @@ -14,11 +14,18 @@ static int cachefiles_ondemand_fd_release(struct inode *inode, >> struct file *file) >> { >> struct cachefiles_object *object = file->private_data; >> - struct cachefiles_cache *cache = object->volume->cache; >> - struct cachefiles_ondemand_info *info = object->ondemand; >> + struct cachefiles_cache *cache; >> + struct cachefiles_ondemand_info *info; >> int object_id; >> struct cachefiles_req *req; >> - XA_STATE(xas, &cache->reqs, 0); >> + XA_STATE(xas, NULL, 0); >> + >> + if (!object) >> + return 0; >> + >> + info = object->ondemand; >> + cache = object->volume->cache; >> + xas.xa = &cache->reqs; >> >> xa_lock(&cache->reqs); >> spin_lock(&info->lock); >> @@ -288,22 +295,39 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req) >> goto err_put_fd; >> } >> >> + spin_lock(&object->ondemand->lock); >> + if (object->ondemand->ondemand_id > 0) { >> + spin_unlock(&object->ondemand->lock); >> + /* Pair with check in cachefiles_ondemand_fd_release(). */ >> + file->private_data = NULL; >> + ret = -EEXIST; >> + goto err_put_file; >> + } >> + >> file->f_mode |= FMODE_PWRITE | FMODE_LSEEK; >> fd_install(fd, file); >> >> load = (void *)req->msg.data; >> load->fd = fd; >> object->ondemand->ondemand_id = object_id; >> + spin_unlock(&object->ondemand->lock); >> >> cachefiles_get_unbind_pincount(cache); >> trace_cachefiles_ondemand_open(object, &req->msg, load); >> return 0; >> >> +err_put_file: >> + fput(file); >> err_put_fd: >> put_unused_fd(fd); >> err_free_id: >> xa_erase(&cache->ondemand_ids, object_id); >> err: >> + spin_lock(&object->ondemand->lock); >> + /* Avoid marking an opened object as closed. */ >> + if (object->ondemand->ondemand_id <= 0) >> + cachefiles_ondemand_set_object_close(object); >> + spin_unlock(&object->ondemand->lock); >> cachefiles_put_object(object, cachefiles_obj_put_ondemand_fd); >> return ret; >> } >> @@ -386,10 +410,8 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache, >> >> if (msg->opcode == CACHEFILES_OP_OPEN) { >> ret = cachefiles_ondemand_get_fd(req); >> - if (ret) { >> - cachefiles_ondemand_set_object_close(req->object); >> + if (ret) >> goto out; >> - } >> } >> >> msg->msg_id = xas.xa_index;
On 5/20/24 5:07 PM, Baokun Li wrote: > On 2024/5/20 16:43, Jingbo Xu wrote: >> >> On 5/15/24 4:45 PM, libaokun@huaweicloud.com wrote: >>> From: Baokun Li <libaokun1@huawei.com> >>> >>> Now every time the daemon reads an open request, it gets a new >>> anonymous fd >>> and ondemand_id. With the introduction of "restore", it is possible >>> to read >>> the same open request more than once, and therefore an object can >>> have more >>> than one anonymous fd. >>> >>> If the anonymous fd is not unique, the following concurrencies will >>> result >>> in an fd leak: >>> >>> t1 | t2 | t3 >>> ------------------------------------------------------------ >>> cachefiles_ondemand_init_object >>> cachefiles_ondemand_send_req >>> REQ_A = kzalloc(sizeof(*req) + data_len) >>> wait_for_completion(&REQ_A->done) >>> cachefiles_daemon_read >>> cachefiles_ondemand_daemon_read >>> REQ_A = cachefiles_ondemand_select_req >>> cachefiles_ondemand_get_fd >>> load->fd = fd0 >>> ondemand_id = object_id0 >>> ------ restore ------ >>> cachefiles_ondemand_restore >>> // restore REQ_A >>> cachefiles_daemon_read >>> cachefiles_ondemand_daemon_read >>> REQ_A = >>> cachefiles_ondemand_select_req >>> cachefiles_ondemand_get_fd >>> load->fd = fd1 >>> ondemand_id = object_id1 >>> process_open_req(REQ_A) >>> write(devfd, ("copen %u,%llu", msg->msg_id, size)) >>> cachefiles_ondemand_copen >>> xa_erase(&cache->reqs, id) >>> complete(&REQ_A->done) >>> kfree(REQ_A) >>> process_open_req(REQ_A) >>> // copen fails due to no req >>> // daemon close(fd1) >>> cachefiles_ondemand_fd_release >>> // set object closed >>> -- umount -- >>> cachefiles_withdraw_cookie >>> cachefiles_ondemand_clean_object >>> cachefiles_ondemand_init_close_req >>> if (!cachefiles_ondemand_object_is_open(object)) >>> return -ENOENT; >>> // The fd0 is not closed until the daemon exits. >>> >>> However, the anonymous fd holds the reference count of the object and >>> the >>> object holds the reference count of the cookie. So even though the >>> cookie >>> has been relinquished, it will not be unhashed and freed until the >>> daemon >>> exits. >>> >>> In fscache_hash_cookie(), when the same cookie is found in the hash >>> list, >>> if the cookie is set with the FSCACHE_COOKIE_RELINQUISHED bit, then >>> the new >>> cookie waits for the old cookie to be unhashed, while the old cookie is >>> waiting for the leaked fd to be closed, if the daemon does not exit >>> in time >>> it will trigger a hung task. >>> >>> To avoid this, allocate a new anonymous fd only if no anonymous fd has >>> been allocated (ondemand_id == 0) or if the previously allocated >>> anonymous >>> fd has been closed (ondemand_id == -1). Moreover, returns an error if >>> ondemand_id is valid, letting the daemon know that the current userland >>> restore logic is abnormal and needs to be checked. >>> >>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking >>> up cookie") >>> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> The LOCs of this fix is quite under control. But still it seems that >> the worst consequence is that the (potential) malicious daemon gets >> hung. No more effect to the system or other processes. Or does a >> non-malicious daemon have any chance having the same issue? > If we enable hung_task_panic, it may cause panic to crash the server. Then this issue has nothing to do with this patch? As long as a malicious daemon doesn't close the anonymous fd after umounting, then I guess a following attempt of mounting cookie with the same name will also wait and hung there?
On 2024/5/20 17:24, Jingbo Xu wrote: > > On 5/20/24 5:07 PM, Baokun Li wrote: >> On 2024/5/20 16:43, Jingbo Xu wrote: >>> On 5/15/24 4:45 PM, libaokun@huaweicloud.com wrote: >>>> From: Baokun Li <libaokun1@huawei.com> >>>> SNIP >>>> >>>> To avoid this, allocate a new anonymous fd only if no anonymous fd has >>>> been allocated (ondemand_id == 0) or if the previously allocated >>>> anonymous >>>> fd has been closed (ondemand_id == -1). Moreover, returns an error if >>>> ondemand_id is valid, letting the daemon know that the current userland >>>> restore logic is abnormal and needs to be checked. >>>> >>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking >>>> up cookie") >>>> Signed-off-by: Baokun Li <libaokun1@huawei.com> >>> The LOCs of this fix is quite under control. But still it seems that >>> the worst consequence is that the (potential) malicious daemon gets >>> hung. No more effect to the system or other processes. Or does a >>> non-malicious daemon have any chance having the same issue? >> If we enable hung_task_panic, it may cause panic to crash the server. > Then this issue has nothing to do with this patch? As long as a > malicious daemon doesn't close the anonymous fd after umounting, then I > guess a following attempt of mounting cookie with the same name will > also wait and hung there? > Yes, a daemon that only reads requests but doesn't process them will cause hung，but the daemon will obey the basic constraints when we test it.
On 2024/5/20 19:14, Baokun Li wrote: > On 2024/5/20 17:24, Jingbo Xu wrote: >> >> On 5/20/24 5:07 PM, Baokun Li wrote: >>> On 2024/5/20 16:43, Jingbo Xu wrote: >>>> On 5/15/24 4:45 PM, libaokun@huaweicloud.com wrote: >>>>> From: Baokun Li <libaokun1@huawei.com> >>>>> > SNIP >>>>> >>>>> To avoid this, allocate a new anonymous fd only if no anonymous fd has >>>>> been allocated (ondemand_id == 0) or if the previously allocated >>>>> anonymous >>>>> fd has been closed (ondemand_id == -1). Moreover, returns an error if >>>>> ondemand_id is valid, letting the daemon know that the current userland >>>>> restore logic is abnormal and needs to be checked. >>>>> >>>>> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking >>>>> up cookie") >>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com> >>>> The LOCs of this fix is quite under control. But still it seems that >>>> the worst consequence is that the (potential) malicious daemon gets >>>> hung. No more effect to the system or other processes. Or does a >>>> non-malicious daemon have any chance having the same issue? >>> If we enable hung_task_panic, it may cause panic to crash the server. >> Then this issue has nothing to do with this patch? As long as a >> malicious daemon doesn't close the anonymous fd after umounting, then I >> guess a following attempt of mounting cookie with the same name will >> also wait and hung there? >> > Yes, a daemon that only reads requests but doesn't process them will > cause hung，but the daemon will obey the basic constraints when we > test it. If we'd really like to enhanace this ("hung_task_panic"), I think you'd better to switch wait_for_completion() to wait_for_completion_killable() at least IMHO anyway. Thanks, Gao Xiang
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c index d04ddc6576e3..d2d4e27fca6f 100644 --- a/fs/cachefiles/ondemand.c +++ b/fs/cachefiles/ondemand.c @@ -14,11 +14,18 @@ static int cachefiles_ondemand_fd_release(struct inode *inode, struct file *file) { struct cachefiles_object *object = file->private_data; - struct cachefiles_cache *cache = object->volume->cache; - struct cachefiles_ondemand_info *info = object->ondemand; + struct cachefiles_cache *cache; + struct cachefiles_ondemand_info *info; int object_id; struct cachefiles_req *req; - XA_STATE(xas, &cache->reqs, 0); + XA_STATE(xas, NULL, 0); + + if (!object) + return 0; + + info = object->ondemand; + cache = object->volume->cache; + xas.xa = &cache->reqs; xa_lock(&cache->reqs); spin_lock(&info->lock); @@ -288,22 +295,39 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req) goto err_put_fd; } + spin_lock(&object->ondemand->lock); + if (object->ondemand->ondemand_id > 0) { + spin_unlock(&object->ondemand->lock); + /* Pair with check in cachefiles_ondemand_fd_release(). */ + file->private_data = NULL; + ret = -EEXIST; + goto err_put_file; + } + file->f_mode |= FMODE_PWRITE | FMODE_LSEEK; fd_install(fd, file); load = (void *)req->msg.data; load->fd = fd; object->ondemand->ondemand_id = object_id; + spin_unlock(&object->ondemand->lock); cachefiles_get_unbind_pincount(cache); trace_cachefiles_ondemand_open(object, &req->msg, load); return 0; +err_put_file: + fput(file); err_put_fd: put_unused_fd(fd); err_free_id: xa_erase(&cache->ondemand_ids, object_id); err: + spin_lock(&object->ondemand->lock); + /* Avoid marking an opened object as closed. */ + if (object->ondemand->ondemand_id <= 0) + cachefiles_ondemand_set_object_close(object); + spin_unlock(&object->ondemand->lock); cachefiles_put_object(object, cachefiles_obj_put_ondemand_fd); return ret; } @@ -386,10 +410,8 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache, if (msg->opcode == CACHEFILES_OP_OPEN) { ret = cachefiles_ondemand_get_fd(req); - if (ret) { - cachefiles_ondemand_set_object_close(req->object); + if (ret) goto out; - } } msg->msg_id = xas.xa_index;