| Message ID | 20190123114912.6487-1-pbutsykin@virtuozzo.com |
|---|---|
| State | New |
| Series | "fs/fuse: move FUSE_S_FAIL_IMMEDIATELY check before kio req send" |
| Headers | show |
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index dc21886ee55d..dcbed5d4932c 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -538,6 +538,11 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, BUG_ON(test_bit(FR_BACKGROUND, &req->flags)); + if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { + req->out.h.error = -EIO; + return; + } + if (fc->kio.op && !fc->kio.op->req_send(fc, req, false, false)) return; @@ -547,11 +552,6 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, req->out.h.error = -ENOTCONN; if (req->end) req->end(fc, req); - } else if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { - spin_unlock(&fiq->waitq.lock); - req->out.h.error = -EIO; - if (req->end) - req->end(fc, req); } else { req->in.h.unique = fuse_get_unique(fiq); queue_request(fiq, req); @@ -627,20 +627,26 @@ void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req) { BUG_ON(!req->end); - if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) - return; - - spin_lock(&fc->lock); if (req->page_cache && req->ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) { - BUG_ON(req->in.h.opcode != FUSE_READ); - req->out.h.error = -EIO; - __clear_bit(FR_BACKGROUND, &req->flags); - __clear_bit(FR_PENDING, &req->flags); - list_del_init(&req->list); - spin_unlock(&fc->lock); - request_end(fc, req); - } else if (fc->connected) { + BUG_ON(req->in.h.opcode != FUSE_READ); + req->out.h.error = -EIO; + __clear_bit(FR_BACKGROUND, &req->flags); + __clear_bit(FR_PENDING, &req->flags); + + spin_lock(&fc->lock); + list_del_init(&req->list); + spin_unlock(&fc->lock); + + request_end(fc, req); + return; + } + + if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) + return; + + spin_lock(&fc->lock); + if (fc->connected) { fuse_request_send_background_locked(fc, req); spin_unlock(&fc->lock); } else {
On 23.01.2019 14:49, Pavel Butsykin wrote: > Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new > requests. But in case of kio requests it doesn't work because the status check > is located behind kio.op->req_send(). To fix this let's move the status check > before kio.op->req_send(). > > Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because > it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> Why is this safe? After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files() may become work not as expected. > --- > fs/fuse/dev.c | 40 +++++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index dc21886ee55d..dcbed5d4932c 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -538,6 +538,11 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, > > BUG_ON(test_bit(FR_BACKGROUND, &req->flags)); > > + if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { > + req->out.h.error = -EIO; > + return; > + } > + > if (fc->kio.op && !fc->kio.op->req_send(fc, req, false, false)) > return; > > @@ -547,11 +552,6 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, > req->out.h.error = -ENOTCONN; > if (req->end) > req->end(fc, req); > - } else if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { > - spin_unlock(&fiq->waitq.lock); > - req->out.h.error = -EIO; > - if (req->end) > - req->end(fc, req); > } else { > req->in.h.unique = fuse_get_unique(fiq); > queue_request(fiq, req); > @@ -627,20 +627,26 @@ void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req) > { > BUG_ON(!req->end); > > - if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) > - return; > - > - spin_lock(&fc->lock); > if (req->page_cache && req->ff && > test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) { > - BUG_ON(req->in.h.opcode != FUSE_READ); > - req->out.h.error = -EIO; > - __clear_bit(FR_BACKGROUND, &req->flags); > - __clear_bit(FR_PENDING, &req->flags); > - list_del_init(&req->list); > - spin_unlock(&fc->lock); > - request_end(fc, req); > - } else if (fc->connected) { > + BUG_ON(req->in.h.opcode != FUSE_READ); > + req->out.h.error = -EIO; > + __clear_bit(FR_BACKGROUND, &req->flags); > + __clear_bit(FR_PENDING, &req->flags); > + > + spin_lock(&fc->lock); > + list_del_init(&req->list); > + spin_unlock(&fc->lock); > + > + request_end(fc, req); > + return; > + } > + > + if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) > + return; > + > + spin_lock(&fc->lock); > + if (fc->connected) { > fuse_request_send_background_locked(fc, req); > spin_unlock(&fc->lock); > } else { >
Ack On Wed, Jan 23, 2019 at 2:49 PM Pavel Butsykin <pbutsykin@virtuozzo.com> wrote: > > Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new > requests. But in case of kio requests it doesn't work because the status check > is located behind kio.op->req_send(). To fix this let's move the status check > before kio.op->req_send(). > > Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because > it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > --- > fs/fuse/dev.c | 40 +++++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index dc21886ee55d..dcbed5d4932c 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -538,6 +538,11 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, > > BUG_ON(test_bit(FR_BACKGROUND, &req->flags)); > > + if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { > + req->out.h.error = -EIO; > + return; > + } > + > if (fc->kio.op && !fc->kio.op->req_send(fc, req, false, false)) > return; > > @@ -547,11 +552,6 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, > req->out.h.error = -ENOTCONN; > if (req->end) > req->end(fc, req); > - } else if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { > - spin_unlock(&fiq->waitq.lock); > - req->out.h.error = -EIO; > - if (req->end) > - req->end(fc, req); > } else { > req->in.h.unique = fuse_get_unique(fiq); > queue_request(fiq, req); > @@ -627,20 +627,26 @@ void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req) > { > BUG_ON(!req->end); > > - if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) > - return; > - > - spin_lock(&fc->lock); > if (req->page_cache && req->ff && > test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) { > - BUG_ON(req->in.h.opcode != FUSE_READ); > - req->out.h.error = -EIO; > - __clear_bit(FR_BACKGROUND, &req->flags); > - __clear_bit(FR_PENDING, &req->flags); > - list_del_init(&req->list); > - spin_unlock(&fc->lock); > - request_end(fc, req); > - } else if (fc->connected) { > + BUG_ON(req->in.h.opcode != FUSE_READ); > + req->out.h.error = -EIO; > + __clear_bit(FR_BACKGROUND, &req->flags); > + __clear_bit(FR_PENDING, &req->flags); > + > + spin_lock(&fc->lock); > + list_del_init(&req->list); > + spin_unlock(&fc->lock); > + > + request_end(fc, req); > + return; > + } > + > + if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) > + return; > + > + spin_lock(&fc->lock); > + if (fc->connected) { > fuse_request_send_background_locked(fc, req); > spin_unlock(&fc->lock); > } else { > -- > 2.15.1 >
23.01.2019 16:55, Kirill Tkhai пишет: > On 23.01.2019 14:49, Pavel Butsykin wrote: >> Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new >> requests. But in case of kio requests it doesn't work because the status check >> is located behind kio.op->req_send(). To fix this let's move the status check >> before kio.op->req_send(). >> >> Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because >> it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). >> >> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > Why is this safe? > > After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files() > may become work not as expected. test_bit is atomic operation. Which type of race do you mean? >> --- >> fs/fuse/dev.c | 40 +++++++++++++++++++++++----------------- >> 1 file changed, 23 insertions(+), 17 deletions(-) >> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c >> index dc21886ee55d..dcbed5d4932c 100644 >> --- a/fs/fuse/dev.c >> +++ b/fs/fuse/dev.c >> @@ -538,6 +538,11 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, >> >> BUG_ON(test_bit(FR_BACKGROUND, &req->flags)); >> >> + if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { >> + req->out.h.error = -EIO; >> + return; >> + } >> + >> if (fc->kio.op && !fc->kio.op->req_send(fc, req, false, false)) >> return; >> >> @@ -547,11 +552,6 @@ static void __fuse_request_send(struct fuse_conn *fc, struct fuse_req *req, >> req->out.h.error = -ENOTCONN; >> if (req->end) >> req->end(fc, req); >> - } else if (ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)) { >> - spin_unlock(&fiq->waitq.lock); >> - req->out.h.error = -EIO; >> - if (req->end) >> - req->end(fc, req); >> } else { >> req->in.h.unique = fuse_get_unique(fiq); >> queue_request(fiq, req); >> @@ -627,20 +627,26 @@ void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req) >> { >> BUG_ON(!req->end); >> >> - if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) >> - return; >> - >> - spin_lock(&fc->lock); >> if (req->page_cache && req->ff && >> test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) { >> - BUG_ON(req->in.h.opcode != FUSE_READ); >> - req->out.h.error = -EIO; >> - __clear_bit(FR_BACKGROUND, &req->flags); >> - __clear_bit(FR_PENDING, &req->flags); >> - list_del_init(&req->list); >> - spin_unlock(&fc->lock); >> - request_end(fc, req); >> - } else if (fc->connected) { >> + BUG_ON(req->in.h.opcode != FUSE_READ); >> + req->out.h.error = -EIO; >> + __clear_bit(FR_BACKGROUND, &req->flags); >> + __clear_bit(FR_PENDING, &req->flags); >> + >> + spin_lock(&fc->lock); >> + list_del_init(&req->list); >> + spin_unlock(&fc->lock); >> + >> + request_end(fc, req); >> + return; >> + } >> + >> + if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false)) >> + return; >> + >> + spin_lock(&fc->lock); >> + if (fc->connected) { >> fuse_request_send_background_locked(fc, req); >> spin_unlock(&fc->lock); >> } else { >>
On 23.01.2019 20:22, Pavel Butsykin wrote: > > 23.01.2019 16:55, Kirill Tkhai пишет: >> On 23.01.2019 14:49, Pavel Butsykin wrote: >>> Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new >>> requests. But in case of kio requests it doesn't work because the status check >>> is located behind kio.op->req_send(). To fix this let's move the status check >>> before kio.op->req_send(). >>> >>> Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because >>> it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). >>> >>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >> Why is this safe? >> >> After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files() >> may become work not as expected. > > test_bit is atomic operation. Which type of race do you mean? fuse_request_send_background() fuse_invalidate_files() test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) spin_lock(&fc->lock); list_for_each_entry(ff, &fi->rw_files, rw_entry) set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state); spin_unlock(&fc->lock); spin_lock(&fc->lock); fuse_kill_requests(fc, inode, &fc->bg_queue); spin_unlock(&fc->lock); spin_lock(&fc->lock); if (fc->connected) { fuse_request_send_background_locked(fc, req); <-- queuing request after fuse_invalidate_files() thinks that requests for all immediate files already killed.
Yes, I missed this synchronization idea, the check and list_add should be together, will fix. On 24.01.2019 11:45, Kirill Tkhai wrote: > On 23.01.2019 20:22, Pavel Butsykin wrote: >> >> 23.01.2019 16:55, Kirill Tkhai пишет: >>> On 23.01.2019 14:49, Pavel Butsykin wrote: >>>> Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new >>>> requests. But in case of kio requests it doesn't work because the status check >>>> is located behind kio.op->req_send(). To fix this let's move the status check >>>> before kio.op->req_send(). >>>> >>>> Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because >>>> it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). >>>> >>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >>> Why is this safe? >>> >>> After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files() >>> may become work not as expected. >> >> test_bit is atomic operation. Which type of race do you mean? > > fuse_request_send_background() fuse_invalidate_files() > test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) > > spin_lock(&fc->lock); > list_for_each_entry(ff, &fi->rw_files, rw_entry) > set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state); > spin_unlock(&fc->lock); > > spin_lock(&fc->lock); > fuse_kill_requests(fc, inode, &fc->bg_queue); > spin_unlock(&fc->lock); > > > spin_lock(&fc->lock); > if (fc->connected) { > fuse_request_send_background_locked(fc, req); <-- queuing request after fuse_invalidate_files() thinks that > requests for all immediate files already killed. > >
On 24.01.2019 12:17, Pavel Butsykin wrote: > Yes, I missed this synchronization idea, the check and list_add should > be together, will fix. Also, __fuse_request_send() may need to be fixed, since it does not look as having appropriate in already existing code (I haven't checked deeply). > On 24.01.2019 11:45, Kirill Tkhai wrote: >> On 23.01.2019 20:22, Pavel Butsykin wrote: >>> >>> 23.01.2019 16:55, Kirill Tkhai пишет: >>>> On 23.01.2019 14:49, Pavel Butsykin wrote: >>>>> Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new >>>>> requests. But in case of kio requests it doesn't work because the status check >>>>> is located behind kio.op->req_send(). To fix this let's move the status check >>>>> before kio.op->req_send(). >>>>> >>>>> Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because >>>>> it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). >>>>> >>>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >>>> Why is this safe? >>>> >>>> After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files() >>>> may become work not as expected. >>> >>> test_bit is atomic operation. Which type of race do you mean? >> >> fuse_request_send_background() fuse_invalidate_files() >> test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) >> >> spin_lock(&fc->lock); >> list_for_each_entry(ff, &fi->rw_files, rw_entry) >> set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state); >> spin_unlock(&fc->lock); >> >> spin_lock(&fc->lock); >> fuse_kill_requests(fc, inode, &fc->bg_queue); >> spin_unlock(&fc->lock); >> >> >> spin_lock(&fc->lock); >> if (fc->connected) { >> fuse_request_send_background_locked(fc, req); <-- queuing request after fuse_invalidate_files() thinks that >> requests for all immediate files already killed. >> >>
On 24.01.2019 12:21, Kirill Tkhai wrote: > On 24.01.2019 12:17, Pavel Butsykin wrote: >> Yes, I missed this synchronization idea, the check and list_add should >> be together, will fix. > > Also, __fuse_request_send() may need to be fixed, since it does not look > as having appropriate in already existing code (I haven't checked deeply). Yep, a similar race is present. Moreover, there may be kernel panic in fuse_kill_requests(fc, inode, &fiq->pending) since fiq->pending list is passed without fiq->waitq.lock. > >> On 24.01.2019 11:45, Kirill Tkhai wrote: >>> On 23.01.2019 20:22, Pavel Butsykin wrote: >>>> >>>> 23.01.2019 16:55, Kirill Tkhai пишет: >>>>> On 23.01.2019 14:49, Pavel Butsykin wrote: >>>>>> Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new >>>>>> requests. But in case of kio requests it doesn't work because the status check >>>>>> is located behind kio.op->req_send(). To fix this let's move the status check >>>>>> before kio.op->req_send(). >>>>>> >>>>>> Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because >>>>>> it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). >>>>>> >>>>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >>>>> Why is this safe? >>>>> >>>>> After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files() >>>>> may become work not as expected. >>>> >>>> test_bit is atomic operation. Which type of race do you mean? >>> >>> fuse_request_send_background() fuse_invalidate_files() >>> test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) >>> >>> spin_lock(&fc->lock); >>> list_for_each_entry(ff, &fi->rw_files, rw_entry) >>> set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state); >>> spin_unlock(&fc->lock); >>> >>> spin_lock(&fc->lock); >>> fuse_kill_requests(fc, inode, &fc->bg_queue); >>> spin_unlock(&fc->lock); >>> >>> >>> spin_lock(&fc->lock); >>> if (fc->connected) { >>> fuse_request_send_background_locked(fc, req); <-- queuing request after fuse_invalidate_files() thinks that >>> requests for all immediate files already killed. >>> >>>
Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new requests. But in case of kio requests it doesn't work because the status check is located behind kio.op->req_send(). To fix this let's move the status check before kio.op->req_send(). Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because it was only needed to clenup kio setattr request after pcs_kio_setattr_handle(). Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- fs/fuse/dev.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-)