| Message ID | 20220113153249.710216-1-sebastian.hasler@stuvus.uni-stuttgart.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] virtiofsd: Do not support blocking flock | expand |
On Thu, Jan 13, 2022 at 04:32:49PM +0100, Sebastian Hasler wrote: > With the current implementation, blocking flock can lead to > deadlock. Thus, it's better to return EOPNOTSUPP if a user attempts > to perform a blocking flock request. > > Signed-off-by: Sebastian Hasler <sebastian.hasler@stuvus.uni-stuttgart.de> Reviewed-by: Vivek Goyal <vgoyal@redhat.com> Thanks Sebastian. Good fix. I can easily reproduce the deadlock. shell1> flock foo.txt -c "sleep 10" shell2> flock foo.txt -c echo First commands take flock on foo.txt. Second command blocks on lock. And only virtiofsd thread serving the virt messages blocks on flock(). Now first command never exits. I think it will try to free lock once sleep is over and that will deadlock. virtiofsd thread is blocked and it will never wake up because lock release operation will never make progress. This will be little painful for people as they will start seeing errors. But I guess erroring out early is better than a potential deadlock later. Vivek > --- > tools/virtiofsd/passthrough_ll.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 64b5b4fbb1..faa62278c5 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -2442,6 +2442,15 @@ static void lo_flock(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi, > int res; > (void)ino; > > + if (!(op & LOCK_NB)) { > + /* > + * Blocking flock can deadlock as there is only one thread > + * serving the queue. > + */ > + fuse_reply_err(req, EOPNOTSUPP); > + return; > + } > + > res = flock(lo_fi_fd(req, fi), op); > > fuse_reply_err(req, res == -1 ? errno : 0); > -- > 2.33.1 > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://listman.redhat.com/mailman/listinfo/virtio-fs >
On Thu, 13 Jan 2022 16:32:49 +0100 Sebastian Hasler <sebastian.hasler@stuvus.uni-stuttgart.de> wrote: > With the current implementation, blocking flock can lead to > deadlock. Thus, it's better to return EOPNOTSUPP if a user attempts > to perform a blocking flock request. > > Signed-off-by: Sebastian Hasler <sebastian.hasler@stuvus.uni-stuttgart.de> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > tools/virtiofsd/passthrough_ll.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 64b5b4fbb1..faa62278c5 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -2442,6 +2442,15 @@ static void lo_flock(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi, > int res; > (void)ino; > > + if (!(op & LOCK_NB)) { > + /* > + * Blocking flock can deadlock as there is only one thread > + * serving the queue. > + */ > + fuse_reply_err(req, EOPNOTSUPP); > + return; > + } > + > res = flock(lo_fi_fd(req, fi), op); > > fuse_reply_err(req, res == -1 ? errno : 0);
* Sebastian Hasler (sebastian.hasler@stuvus.uni-stuttgart.de) wrote: > With the current implementation, blocking flock can lead to > deadlock. Thus, it's better to return EOPNOTSUPP if a user attempts > to perform a blocking flock request. > > Signed-off-by: Sebastian Hasler <sebastian.hasler@stuvus.uni-stuttgart.de> Queued, apologies for the delay. > --- > tools/virtiofsd/passthrough_ll.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 64b5b4fbb1..faa62278c5 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -2442,6 +2442,15 @@ static void lo_flock(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi, > int res; > (void)ino; > > + if (!(op & LOCK_NB)) { > + /* > + * Blocking flock can deadlock as there is only one thread > + * serving the queue. > + */ > + fuse_reply_err(req, EOPNOTSUPP); > + return; > + } > + > res = flock(lo_fi_fd(req, fi), op); > > fuse_reply_err(req, res == -1 ? errno : 0); > -- > 2.33.1
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 64b5b4fbb1..faa62278c5 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -2442,6 +2442,15 @@ static void lo_flock(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi, int res; (void)ino; + if (!(op & LOCK_NB)) { + /* + * Blocking flock can deadlock as there is only one thread + * serving the queue. + */ + fuse_reply_err(req, EOPNOTSUPP); + return; + } + res = flock(lo_fi_fd(req, fi), op); fuse_reply_err(req, res == -1 ? errno : 0);
With the current implementation, blocking flock can lead to deadlock. Thus, it's better to return EOPNOTSUPP if a user attempts to perform a blocking flock request. Signed-off-by: Sebastian Hasler <sebastian.hasler@stuvus.uni-stuttgart.de> --- tools/virtiofsd/passthrough_ll.c | 9 +++++++++ 1 file changed, 9 insertions(+)