Re: [Virtio-fs] [PATCH] virtiofsd: remove symlink fallbacks
* Vivek Goyal (vgo...@redhat.com) wrote:
> On Thu, May 14, 2020 at 04:07:36PM +0200, Miklos Szeredi wrote:
> > Path lookup in the kernel has special rules for looking up magic symlinks
> > under /proc. If a filesystem operation is instructed to follow symlinks
> > (e.g. via AT_SYMLINK_FOLLOW or lack of AT_SYMLINK_NOFOLLOW), and the final
> > component is such a proc symlink, then the target of the magic symlink is
> > used for the operation, even if the target itself is a symlink. I.e. path
> > lookup is always terminated after following a final magic symlink.
> >
> > I was erronously assuming that in the above case the target symlink would
> > also be followed, and so workarounds were added for a couple of operations
> > to handle the symlink case. Since the symlink can be handled simply by
> > following the proc symlink, these workardouds are not needed.
> >
> > Also remove the "norace" option, which disabled the workarounds.
> >
> > Commit bdfd66788349 ("virtiofsd: Fix xattr operations") already dealt with
> > the same issue for xattr operations.
> >
> > Signed-off-by: Miklos Szeredi
>
> Good to have this cleanup.
>
> Acked-by: Vivek Goyal
Queued.
> Vivek
>
> > ---
> > tools/virtiofsd/passthrough_ll.c | 175 ++-
> > 1 file changed, 6 insertions(+), 169 deletions(-)
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c
> > b/tools/virtiofsd/passthrough_ll.c
> > index 3ba1d9098460..2ce7c96085bf 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -140,7 +140,6 @@ enum {
> > struct lo_data {
> > pthread_mutex_t mutex;
> > int debug;
> > -int norace;
> > int writeback;
> > int flock;
> > int posix_lock;
> > @@ -176,7 +175,6 @@ static const struct fuse_opt lo_opts[] = {
> > { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE },
> > { "cache=auto", offsetof(struct lo_data, cache), CACHE_AUTO },
> > { "cache=always", offsetof(struct lo_data, cache), CACHE_ALWAYS },
> > -{ "norace", offsetof(struct lo_data, norace), 1 },
> > { "readdirplus", offsetof(struct lo_data, readdirplus_set), 1 },
> > { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 },
> > FUSE_OPT_END
> > @@ -592,136 +590,6 @@ static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
> > fuse_reply_attr(req, , lo->timeout);
> > }
> >
> > -/*
> > - * Increments parent->nlookup and caller must release refcount using
> > - * lo_inode_put().
> > - */
> > -static int lo_parent_and_name(struct lo_data *lo, struct lo_inode *inode,
> > - char path[PATH_MAX], struct lo_inode
> > **parent)
> > -{
> > -char procname[64];
> > -char *last;
> > -struct stat stat;
> > -struct lo_inode *p;
> > -int retries = 2;
> > -int res;
> > -
> > -retry:
> > -sprintf(procname, "%i", inode->fd);
> > -
> > -res = readlinkat(lo->proc_self_fd, procname, path, PATH_MAX);
> > -if (res < 0) {
> > -fuse_log(FUSE_LOG_WARNING, "%s: readlink failed: %m\n", __func__);
> > -goto fail_noretry;
> > -}
> > -
> > -if (res >= PATH_MAX) {
> > -fuse_log(FUSE_LOG_WARNING, "%s: readlink overflowed\n", __func__);
> > -goto fail_noretry;
> > -}
> > -path[res] = '\0';
> > -
> > -last = strrchr(path, '/');
> > -if (last == NULL) {
> > -/* Shouldn't happen */
> > -fuse_log(
> > -FUSE_LOG_WARNING,
> > -"%s: INTERNAL ERROR: bad path read from proc\n", __func__);
> > -goto fail_noretry;
> > -}
> > -if (last == path) {
> > -p = >root;
> > -pthread_mutex_lock(>mutex);
> > -p->nlookup++;
> > -g_atomic_int_inc(>refcount);
> > -pthread_mutex_unlock(>mutex);
> > -} else {
> > -*last = '\0';
> > -res = fstatat(AT_FDCWD, last == path ? "/" : path, , 0);
> > -if (res == -1) {
> > -if (!retries) {
> > -fuse_log(FUSE_LOG_WARNING,
> > - "%s: failed to stat parent: %m\n", __func__);
> > -}
> > -goto fail;
> > -}
> > -p = lo_find(lo, );
> > -if (p == NULL) {
> > -if (!retries) {
> > -fuse_log(FUSE_LOG_WARNING,
> > - "%s: failed to find parent\n", __func__);
> > -}
> > -goto fail;
> > -}
> > -}
> > -last++;
> > -res = fstatat(p->fd, last, , AT_SYMLINK_NOFOLLOW);
> > -if (res == -1) {
> > -if (!retries) {
> > -fuse_log(FUSE_LOG_WARNING,
> > - "%s: failed to stat last\n", __func__);
> > -}
> > -goto fail_unref;
> > -}
> > -if (stat.st_dev != inode->key.dev || stat.st_ino != inode->key.ino) {
> > -if (!retries) {
> > -fuse_log(FUSE_LOG_WARNING,
> > - "%s: failed to match last\n", __func__);
> > -
Re: [Virtio-fs] [PATCH] virtiofsd: remove symlink fallbacks
On Thu, May 14, 2020 at 04:07:36PM +0200, Miklos Szeredi wrote:
> Path lookup in the kernel has special rules for looking up magic symlinks
> under /proc. If a filesystem operation is instructed to follow symlinks
> (e.g. via AT_SYMLINK_FOLLOW or lack of AT_SYMLINK_NOFOLLOW), and the final
> component is such a proc symlink, then the target of the magic symlink is
> used for the operation, even if the target itself is a symlink. I.e. path
> lookup is always terminated after following a final magic symlink.
>
> I was erronously assuming that in the above case the target symlink would
> also be followed, and so workarounds were added for a couple of operations
> to handle the symlink case. Since the symlink can be handled simply by
> following the proc symlink, these workardouds are not needed.
>
> Also remove the "norace" option, which disabled the workarounds.
>
> Commit bdfd66788349 ("virtiofsd: Fix xattr operations") already dealt with
> the same issue for xattr operations.
>
> Signed-off-by: Miklos Szeredi
Good to have this cleanup.
Acked-by: Vivek Goyal
Vivek
> ---
> tools/virtiofsd/passthrough_ll.c | 175 ++-
> 1 file changed, 6 insertions(+), 169 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index 3ba1d9098460..2ce7c96085bf 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -140,7 +140,6 @@ enum {
> struct lo_data {
> pthread_mutex_t mutex;
> int debug;
> -int norace;
> int writeback;
> int flock;
> int posix_lock;
> @@ -176,7 +175,6 @@ static const struct fuse_opt lo_opts[] = {
> { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE },
> { "cache=auto", offsetof(struct lo_data, cache), CACHE_AUTO },
> { "cache=always", offsetof(struct lo_data, cache), CACHE_ALWAYS },
> -{ "norace", offsetof(struct lo_data, norace), 1 },
> { "readdirplus", offsetof(struct lo_data, readdirplus_set), 1 },
> { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 },
> FUSE_OPT_END
> @@ -592,136 +590,6 @@ static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
> fuse_reply_attr(req, , lo->timeout);
> }
>
> -/*
> - * Increments parent->nlookup and caller must release refcount using
> - * lo_inode_put().
> - */
> -static int lo_parent_and_name(struct lo_data *lo, struct lo_inode *inode,
> - char path[PATH_MAX], struct lo_inode **parent)
> -{
> -char procname[64];
> -char *last;
> -struct stat stat;
> -struct lo_inode *p;
> -int retries = 2;
> -int res;
> -
> -retry:
> -sprintf(procname, "%i", inode->fd);
> -
> -res = readlinkat(lo->proc_self_fd, procname, path, PATH_MAX);
> -if (res < 0) {
> -fuse_log(FUSE_LOG_WARNING, "%s: readlink failed: %m\n", __func__);
> -goto fail_noretry;
> -}
> -
> -if (res >= PATH_MAX) {
> -fuse_log(FUSE_LOG_WARNING, "%s: readlink overflowed\n", __func__);
> -goto fail_noretry;
> -}
> -path[res] = '\0';
> -
> -last = strrchr(path, '/');
> -if (last == NULL) {
> -/* Shouldn't happen */
> -fuse_log(
> -FUSE_LOG_WARNING,
> -"%s: INTERNAL ERROR: bad path read from proc\n", __func__);
> -goto fail_noretry;
> -}
> -if (last == path) {
> -p = >root;
> -pthread_mutex_lock(>mutex);
> -p->nlookup++;
> -g_atomic_int_inc(>refcount);
> -pthread_mutex_unlock(>mutex);
> -} else {
> -*last = '\0';
> -res = fstatat(AT_FDCWD, last == path ? "/" : path, , 0);
> -if (res == -1) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to stat parent: %m\n", __func__);
> -}
> -goto fail;
> -}
> -p = lo_find(lo, );
> -if (p == NULL) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to find parent\n", __func__);
> -}
> -goto fail;
> -}
> -}
> -last++;
> -res = fstatat(p->fd, last, , AT_SYMLINK_NOFOLLOW);
> -if (res == -1) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to stat last\n", __func__);
> -}
> -goto fail_unref;
> -}
> -if (stat.st_dev != inode->key.dev || stat.st_ino != inode->key.ino) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to match last\n", __func__);
> -}
> -goto fail_unref;
> -}
> -*parent = p;
> -memmove(path, last, strlen(last) + 1);
> -
> -return 0;
> -
> -fail_unref:
> -unref_inode_lolocked(lo, p, 1);
> -lo_inode_put(lo, );
> -fail:
> -if (retries) {
> -retries--;
> -goto retry;
> -}
> -fail_noretry:
>
Re: [Virtio-fs] [PATCH] virtiofsd: remove symlink fallbacks
On Fri, May 15, 2020 at 5:43 AM Liu Bo wrote: > > On Thu, May 14, 2020 at 04:07:36PM +0200, Miklos Szeredi wrote: > > Path lookup in the kernel has special rules for looking up magic symlinks > > under /proc. If a filesystem operation is instructed to follow symlinks > > (e.g. via AT_SYMLINK_FOLLOW or lack of AT_SYMLINK_NOFOLLOW), and the final > > component is such a proc symlink, then the target of the magic symlink is > > used for the operation, even if the target itself is a symlink. I.e. path > > lookup is always terminated after following a final magic symlink. > > > > Hi Miklos, > > Are these mentioned special rules supported by a recent kernel version > or are they there from day one linux? Hi Liubo, AFAICS, these rules have been there from day one. Thanks, Miklos
Re: [Virtio-fs] [PATCH] virtiofsd: remove symlink fallbacks
On Thu, May 14, 2020 at 04:07:36PM +0200, Miklos Szeredi wrote:
> Path lookup in the kernel has special rules for looking up magic symlinks
> under /proc. If a filesystem operation is instructed to follow symlinks
> (e.g. via AT_SYMLINK_FOLLOW or lack of AT_SYMLINK_NOFOLLOW), and the final
> component is such a proc symlink, then the target of the magic symlink is
> used for the operation, even if the target itself is a symlink. I.e. path
> lookup is always terminated after following a final magic symlink.
>
Hi Miklos,
Are these mentioned special rules supported by a recent kernel version
or are they there from day one linux?
thanks,
liubo
> I was erronously assuming that in the above case the target symlink would
> also be followed, and so workarounds were added for a couple of operations
> to handle the symlink case. Since the symlink can be handled simply by
> following the proc symlink, these workardouds are not needed.
>
> Also remove the "norace" option, which disabled the workarounds.
>
> Commit bdfd66788349 ("virtiofsd: Fix xattr operations") already dealt with
> the same issue for xattr operations.
>
> Signed-off-by: Miklos Szeredi
> ---
> tools/virtiofsd/passthrough_ll.c | 175 ++-
> 1 file changed, 6 insertions(+), 169 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index 3ba1d9098460..2ce7c96085bf 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -140,7 +140,6 @@ enum {
> struct lo_data {
> pthread_mutex_t mutex;
> int debug;
> -int norace;
> int writeback;
> int flock;
> int posix_lock;
> @@ -176,7 +175,6 @@ static const struct fuse_opt lo_opts[] = {
> { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE },
> { "cache=auto", offsetof(struct lo_data, cache), CACHE_AUTO },
> { "cache=always", offsetof(struct lo_data, cache), CACHE_ALWAYS },
> -{ "norace", offsetof(struct lo_data, norace), 1 },
> { "readdirplus", offsetof(struct lo_data, readdirplus_set), 1 },
> { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 },
> FUSE_OPT_END
> @@ -592,136 +590,6 @@ static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
> fuse_reply_attr(req, , lo->timeout);
> }
>
> -/*
> - * Increments parent->nlookup and caller must release refcount using
> - * lo_inode_put().
> - */
> -static int lo_parent_and_name(struct lo_data *lo, struct lo_inode *inode,
> - char path[PATH_MAX], struct lo_inode **parent)
> -{
> -char procname[64];
> -char *last;
> -struct stat stat;
> -struct lo_inode *p;
> -int retries = 2;
> -int res;
> -
> -retry:
> -sprintf(procname, "%i", inode->fd);
> -
> -res = readlinkat(lo->proc_self_fd, procname, path, PATH_MAX);
> -if (res < 0) {
> -fuse_log(FUSE_LOG_WARNING, "%s: readlink failed: %m\n", __func__);
> -goto fail_noretry;
> -}
> -
> -if (res >= PATH_MAX) {
> -fuse_log(FUSE_LOG_WARNING, "%s: readlink overflowed\n", __func__);
> -goto fail_noretry;
> -}
> -path[res] = '\0';
> -
> -last = strrchr(path, '/');
> -if (last == NULL) {
> -/* Shouldn't happen */
> -fuse_log(
> -FUSE_LOG_WARNING,
> -"%s: INTERNAL ERROR: bad path read from proc\n", __func__);
> -goto fail_noretry;
> -}
> -if (last == path) {
> -p = >root;
> -pthread_mutex_lock(>mutex);
> -p->nlookup++;
> -g_atomic_int_inc(>refcount);
> -pthread_mutex_unlock(>mutex);
> -} else {
> -*last = '\0';
> -res = fstatat(AT_FDCWD, last == path ? "/" : path, , 0);
> -if (res == -1) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to stat parent: %m\n", __func__);
> -}
> -goto fail;
> -}
> -p = lo_find(lo, );
> -if (p == NULL) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to find parent\n", __func__);
> -}
> -goto fail;
> -}
> -}
> -last++;
> -res = fstatat(p->fd, last, , AT_SYMLINK_NOFOLLOW);
> -if (res == -1) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to stat last\n", __func__);
> -}
> -goto fail_unref;
> -}
> -if (stat.st_dev != inode->key.dev || stat.st_ino != inode->key.ino) {
> -if (!retries) {
> -fuse_log(FUSE_LOG_WARNING,
> - "%s: failed to match last\n", __func__);
> -}
> -goto fail_unref;
> -}
> -*parent = p;
> -memmove(path, last, strlen(last) + 1);
> -
> -return 0;
> -
> -fail_unref:
> -unref_inode_lolocked(lo, p, 1);
> -lo_inode_put(lo, );
> -fail:
> -if
