| Message ID | 20180223120106.32921-1-yangerkun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 23, 2018 at 2:01 PM, yangerkun <yangerkun@huawei.com> wrote: > For origin xattr, use fid in ovl_fh to consume file_handle, and use > open_by_handle_at to check validity of the origin ovl_fh. In auto mode, > the invalid origin xattr will remove automatic. > > Signed-off-by: yangerkun <yangerkun@huawei.com> > --- > check.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > lib.c | 10 +++++ > lib.h | 4 +- > overlayfs.c | 23 +++++++++++ > overlayfs.h | 33 +++++++++++++++ > 5 files changed, 202 insertions(+), 3 deletions(-) > > diff --git a/check.c b/check.c > index e109620..50c4585 100644 > --- a/check.c > +++ b/check.c > @@ -24,6 +24,7 @@ > > #include <stdlib.h> > #include <stdio.h> > +#include <stddef.h> > #include <string.h> > #include <errno.h> > #include <unistd.h> > @@ -169,11 +170,42 @@ static int ovl_get_redirect(int dirfd, const char *pathname, > return 0; > } > > +/* > + * Get the origin file handle with the pathname, then check the validity of member > + * in ovl_fh. > + */ > +static int ovl_get_fh(int dirfd, const char *pathname, > + struct ovl_fh **fh, bool *invalid) > +{ > + char *og = NULL; > + ssize_t ret; > + > + ret = get_xattr(dirfd, pathname, OVL_ORIGIN_XATTR, &og, NULL); > + if (ret <= 0) > + return ret; > + > + ret = ovl_check_fh_len((struct ovl_fh *)og, ret); > + if (ret) { > + free(og); > + if (ret == -EINVAL) > + *invalid = true; > + } else { > + *fh = (struct ovl_fh *)og; > + } > + > + return 0; > +} > + > static inline int ovl_remove_redirect(int dirfd, const char *pathname) > { > return remove_xattr(dirfd, pathname, OVL_REDIRECT_XATTR); > } > > +static inline int ovl_remove_origin(int dirfd, const char *pathname) > +{ > + return remove_xattr(dirfd, pathname, OVL_ORIGIN_XATTR); > +} > + > static inline int ovl_create_whiteout(int dirfd, const char *pathname) > { > if (mknodat(dirfd, pathname, S_IFCHR | WHITEOUT_MOD, makedev(0, 0))) { > @@ -758,6 +790,97 @@ out: > } > > /* > + * Get origin file handle stored in the xattr, check it's invalid > + * or not. In auto-mode, invalid origin xattr will be removed directly. > + */ > +static int ovl_check_origin(struct scan_ctx *sctx) > +{ > + struct ovl_fh *fh = NULL; > + struct file_handle *fhp = NULL; > + struct stat stat; > + bool invalid = false; > + int i; > + int ret; > + int fid_len; > + int fd; > + > + /* Get origin */ > + ret = ovl_get_fh(sctx->dirfd, sctx->pathname, &fh, &invalid); > + if (ret < 0) > + return -1; > + > + /* Cannot get ovl_fh, maybe not set or unrecognized */ Should fsck issue a warning about this? Separate the case of not set from unrecognized? At least leave a TODO comment here of what fsck should do in those cases. > + if (!fh && !invalid) > + return 0; > + > + print_debug(_("File or dir \"%s\" has origin\n"), sctx->pathname); > + sctx->t_origins++; > + > + /* ovl_fh is invalid */ > + if (invalid) > + goto remove; > + > + /* Origin file in last lower dir? */ > + if (sctx->dirtype == OVL_LOWER && sctx->stack == lower_num -1) > + goto remove; > + > + fid_len = fh->len - offsetof(struct ovl_fh, fid); > + fhp = smalloc(sizeof(struct file_handle) + fid_len); > + fhp->handle_bytes = fh->len - offsetof(struct ovl_fh, fid); > + fhp->handle_type = fh->type; > + memcpy(fhp->f_handle, fh->fid, fid_len); > + > + /* Scan lower directories to check origin xattr's validity */ > + i = sctx->dirtype == OVL_UPPER ? 0 : sctx->stack + 1; > + for (; i < lower_num; i++) { > + fd = open_by_handle_at(lowerfd[i], fhp, O_RDONLY); > + > + if (fd == -1) { > + if (errno == ESTALE || errno == ENOENT) ENOENT from open_by_handle_at() is not relevant to this code. > + continue; > + > + print_err(_("Error open_by_handle_at %s:%s\n"), > + sctx->pathname, strerror(errno)); > + ret = -1; > + goto out; > + } > + > + ret = fstat(fd, &stat); > + if (ret) { > + print_err(_("Cannot fstat origin file in %s of %s:%s\n"), > + lowerdir[i], sctx->pathname, strerror(errno)); > + goto out2; > + } > + > + if (stat.st_mode != sctx->st->st_mode) > + goto remove; > + > + goto out2; > + } > + > +remove: > + sctx->i_origins++; > + > + /* Remove origin xattr or ask user */ > + if (!ovl_ask_action("Invalid origin xattr", sctx->pathname, sctx->dirtype, > + sctx->stack, "Remove origin", 1)) > + goto out; > + > + ret = ovl_remove_origin(sctx->dirfd, sctx->pathname); > + if (ret) > + goto out; > + > + sctx->i_origins--; > + sctx->t_origins--; > +out2: > + close(fd); > +out: > + free(fhp); > + free(fh); > + return ret; > +} > + > +/* > * If a directory has origin target and redirect/merge subdirectories in it, > * it may contain copied up targets. In order to avoid 'd_ino' change after > * lower target copy-up or rename (which will create a new inode), > @@ -870,9 +993,11 @@ static struct scan_operations ovl_scan_ops[OVL_SCAN_PASS][2] = { > .whiteout = ovl_check_whiteout, > .impurity = ovl_count_impurity, > .impure = ovl_check_impure, > + .origin = ovl_check_origin, > }, > [OVL_LOWER] = { > .whiteout = ovl_check_whiteout, > + .origin = ovl_check_origin, > }, > }/* Pass Two */ > }; > @@ -892,11 +1017,12 @@ static void ovl_scan_report(struct scan_ctx *sctx) > { > if (flags & FL_VERBOSE) { > print_info(_("Scan %d directories, %d files, " > - "%d/%d whiteouts, %d/%d redirect dirs " > - "%d missing impure\n"), > + "%d/%d whiteouts, %d/%d redirect dirs, " > + "%d/%d origins, %d missing impure\n"), > sctx->directories, sctx->files, > sctx->i_whiteouts, sctx->t_whiteouts, > sctx->i_redirects, sctx->t_redirects, > + sctx->i_origins, sctx->t_origins, > sctx->m_impure); > } > } > @@ -916,6 +1042,11 @@ static void ovl_scan_check(struct scan_ctx *sctx) > sctx->i_redirects); > incons = true; > } > + if (sctx->i_origins) { > + print_info(_("Invalid origins %d left!\n"), > + sctx->i_origins); > + incons = true; > + } > if (sctx->m_impure) { > print_info(_("Directories %d missing impure xattr!\n"), > sctx->m_impure); > diff --git a/lib.c b/lib.c > index 4d9536a..2aee0dc 100644 > --- a/lib.c > +++ b/lib.c > @@ -244,6 +244,11 @@ int scan_dir(struct scan_ctx *sctx, struct scan_operations *sop) > ret = scan_check_entry(sop->impurity, sctx); > if (ret) > goto out; > + > + /* Check origins */ > + ret = scan_check_entry(sop->origin, sctx); > + if (ret) > + goto out; > break; > case FTS_DEFAULT: > /* Check whiteouts */ > @@ -264,6 +269,11 @@ int scan_dir(struct scan_ctx *sctx, struct scan_operations *sop) > if (ret) > goto out; > > + /* Check origins */ > + ret = scan_check_entry(sop->origin, sctx); > + if (ret) > + goto out; > + > /* Save current dir data and create new one for subdir */ > ftsent->fts_pointer = sctx->dirdata; > sctx->dirdata = smalloc(sizeof(struct scan_dir_data)); > diff --git a/lib.h b/lib.h > index 880a8c3..5f7fdaa 100644 > --- a/lib.h > +++ b/lib.h > @@ -71,7 +71,9 @@ struct scan_ctx { > int i_whiteouts; /* invalid whiteouts */ > int t_redirects; /* total redirect dirs */ > int i_redirects; /* invalid redirect dirs */ > - int m_impure; /* missing inpure dirs */ > + int t_origins; /* total origins */ > + int i_origins; /* invalid origins */ > + int m_impure; /* missing impure dirs */ > > const char *pathname; /* path relative to overlay root */ > const char *filename; /* filename */ > diff --git a/overlayfs.c b/overlayfs.c > index 93b4cad..c2f0840 100644 > --- a/overlayfs.c > +++ b/overlayfs.c > @@ -19,6 +19,8 @@ > */ > > #include <stddef.h> > +#include <errno.h> > +#include "overlayfs.h" > > /* > * Split directories to individual one. > @@ -70,3 +72,24 @@ char *ovl_next_opt(char **s) > *s = NULL; > return sbegin; > } > + > +/* > + * Check the validity of ovl_fh. > + */ > +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len) > +{ > + if (fh_len < sizeof(struct ovl_fh) || fh_len < fh->len) > + return -EINVAL; > + > + if (fh->magic != OVL_FH_MAGIC) > + return -EINVAL; > + > + if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL) > + return -ENODATA; > + > + if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) && > + (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN) > + return -ENODATA; > + Overlayfs approach to unrecognized ovl_fh on-disk format is very naiive borderline buggy. When endian/flags/version check fails, origin is treated as "unknown" and that is fine as long as the only implication is change of inode number. But when checking that origin of upper root matches lower root for index feature, treating unrecognized ovl_fh format as unknown will result in overwriting the root origin and as far as I can tell, this is a bug. The situation with fsck.overlayfs is even worse IMO, because running an old fsck tools is even more easy to get wrong than mounting an overlayfs with an old kernel. In general fsck.* tools, like e2fsck, are more prudent about fixing a filesystem with unknown on-disk format than the kernel about mounting filesystem with unknown on-disk format features. This patch specifically does not seem to do any harm when encountering unknown on-disk format of ovl_fh, but I think fsck.overlay should take some extra steps for safety, before fixing overlayfs with features that fsck.overlay does not support. I suggest at least the following safely checks on start of fsck.overlay: - Check if index dir exists in workdir - Check if upperdir root has an unrecognized origin xattr format - Check if index dir root has an unrecognized origin xattr format If any of the checks above is positive, fsck.overlay should issue a warning and refuse to fix fielsystem. Running with -n -f is allowed. See if index dir exists, then removing origin xattr can corrupt the index. When fsck.overlay gains the knowledge about checking and fixing index, user would opt-in to checking an overlayfs with index by -o index=on. Thanks, Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/24/2018 3:43 PM, Amir Goldstein wrote: > On Fri, Feb 23, 2018 at 2:01 PM, yangerkun <yangerkun@huawei.com> wrote: >> For origin xattr, use fid in ovl_fh to consume file_handle, and use >> open_by_handle_at to check validity of the origin ovl_fh. In auto mode, >> the invalid origin xattr will remove automatic. >> >> Signed-off-by: yangerkun <yangerkun@huawei.com> >> --- >> check.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> lib.c | 10 +++++ >> lib.h | 4 +- >> overlayfs.c | 23 +++++++++++ >> overlayfs.h | 33 +++++++++++++++ >> 5 files changed, 202 insertions(+), 3 deletions(-) >> >> diff --git a/check.c b/check.c >> index e109620..50c4585 100644 >> --- a/check.c >> +++ b/check.c >> @@ -24,6 +24,7 @@ >> >> #include <stdlib.h> >> #include <stdio.h> >> +#include <stddef.h> >> #include <string.h> >> #include <errno.h> >> #include <unistd.h> >> @@ -169,11 +170,42 @@ static int ovl_get_redirect(int dirfd, const char *pathname, >> return 0; >> } >> >> +/* >> + * Get the origin file handle with the pathname, then check the validity of member >> + * in ovl_fh. >> + */ >> +static int ovl_get_fh(int dirfd, const char *pathname, >> + struct ovl_fh **fh, bool *invalid) >> +{ >> + char *og = NULL; >> + ssize_t ret; >> + >> + ret = get_xattr(dirfd, pathname, OVL_ORIGIN_XATTR, &og, NULL); >> + if (ret <= 0) >> + return ret; >> + >> + ret = ovl_check_fh_len((struct ovl_fh *)og, ret); >> + if (ret) { >> + free(og); >> + if (ret == -EINVAL) >> + *invalid = true; >> + } else { >> + *fh = (struct ovl_fh *)og; >> + } >> + >> + return 0; >> +} >> + >> static inline int ovl_remove_redirect(int dirfd, const char *pathname) >> { >> return remove_xattr(dirfd, pathname, OVL_REDIRECT_XATTR); >> } >> >> +static inline int ovl_remove_origin(int dirfd, const char *pathname) >> +{ >> + return remove_xattr(dirfd, pathname, OVL_ORIGIN_XATTR); >> +} >> + >> static inline int ovl_create_whiteout(int dirfd, const char *pathname) >> { >> if (mknodat(dirfd, pathname, S_IFCHR | WHITEOUT_MOD, makedev(0, 0))) { >> @@ -758,6 +790,97 @@ out: >> } >> >> /* >> + * Get origin file handle stored in the xattr, check it's invalid >> + * or not. In auto-mode, invalid origin xattr will be removed directly. >> + */ >> +static int ovl_check_origin(struct scan_ctx *sctx) >> +{ >> + struct ovl_fh *fh = NULL; >> + struct file_handle *fhp = NULL; >> + struct stat stat; >> + bool invalid = false; >> + int i; >> + int ret; >> + int fid_len; >> + int fd; >> + >> + /* Get origin */ >> + ret = ovl_get_fh(sctx->dirfd, sctx->pathname, &fh, &invalid); >> + if (ret < 0) >> + return -1; >> + >> + /* Cannot get ovl_fh, maybe not set or unrecognized */ > > Should fsck issue a warning about this? > Separate the case of not set from unrecognized? > At least leave a TODO comment here of what fsck should do in those cases. > >> + if (!fh && !invalid) >> + return 0; >> + >> + print_debug(_("File or dir \"%s\" has origin\n"), sctx->pathname); >> + sctx->t_origins++; >> + >> + /* ovl_fh is invalid */ >> + if (invalid) >> + goto remove; >> + >> + /* Origin file in last lower dir? */ >> + if (sctx->dirtype == OVL_LOWER && sctx->stack == lower_num -1) >> + goto remove; >> + >> + fid_len = fh->len - offsetof(struct ovl_fh, fid); >> + fhp = smalloc(sizeof(struct file_handle) + fid_len); >> + fhp->handle_bytes = fh->len - offsetof(struct ovl_fh, fid); >> + fhp->handle_type = fh->type; >> + memcpy(fhp->f_handle, fh->fid, fid_len); >> + >> + /* Scan lower directories to check origin xattr's validity */ >> + i = sctx->dirtype == OVL_UPPER ? 0 : sctx->stack + 1; >> + for (; i < lower_num; i++) { >> + fd = open_by_handle_at(lowerfd[i], fhp, O_RDONLY); >> + >> + if (fd == -1) { >> + if (errno == ESTALE || errno == ENOENT) > > ENOENT from open_by_handle_at() is not relevant to this code. > >> + continue; >> + >> + print_err(_("Error open_by_handle_at %s:%s\n"), >> + sctx->pathname, strerror(errno)); >> + ret = -1; >> + goto out; >> + } >> + >> + ret = fstat(fd, &stat); >> + if (ret) { >> + print_err(_("Cannot fstat origin file in %s of %s:%s\n"), >> + lowerdir[i], sctx->pathname, strerror(errno)); >> + goto out2; >> + } >> + >> + if (stat.st_mode != sctx->st->st_mode) >> + goto remove; >> + >> + goto out2; >> + } >> + >> +remove: >> + sctx->i_origins++; >> + >> + /* Remove origin xattr or ask user */ >> + if (!ovl_ask_action("Invalid origin xattr", sctx->pathname, sctx->dirtype, >> + sctx->stack, "Remove origin", 1)) >> + goto out; >> + >> + ret = ovl_remove_origin(sctx->dirfd, sctx->pathname); >> + if (ret) >> + goto out; >> + >> + sctx->i_origins--; >> + sctx->t_origins--; >> +out2: >> + close(fd); >> +out: >> + free(fhp); >> + free(fh); >> + return ret; >> +} >> + >> +/* >> * If a directory has origin target and redirect/merge subdirectories in it, >> * it may contain copied up targets. In order to avoid 'd_ino' change after >> * lower target copy-up or rename (which will create a new inode), >> @@ -870,9 +993,11 @@ static struct scan_operations ovl_scan_ops[OVL_SCAN_PASS][2] = { >> .whiteout = ovl_check_whiteout, >> .impurity = ovl_count_impurity, >> .impure = ovl_check_impure, >> + .origin = ovl_check_origin, >> }, >> [OVL_LOWER] = { >> .whiteout = ovl_check_whiteout, >> + .origin = ovl_check_origin, >> }, >> }/* Pass Two */ >> }; >> @@ -892,11 +1017,12 @@ static void ovl_scan_report(struct scan_ctx *sctx) >> { >> if (flags & FL_VERBOSE) { >> print_info(_("Scan %d directories, %d files, " >> - "%d/%d whiteouts, %d/%d redirect dirs " >> - "%d missing impure\n"), >> + "%d/%d whiteouts, %d/%d redirect dirs, " >> + "%d/%d origins, %d missing impure\n"), >> sctx->directories, sctx->files, >> sctx->i_whiteouts, sctx->t_whiteouts, >> sctx->i_redirects, sctx->t_redirects, >> + sctx->i_origins, sctx->t_origins, >> sctx->m_impure); >> } >> } >> @@ -916,6 +1042,11 @@ static void ovl_scan_check(struct scan_ctx *sctx) >> sctx->i_redirects); >> incons = true; >> } >> + if (sctx->i_origins) { >> + print_info(_("Invalid origins %d left!\n"), >> + sctx->i_origins); >> + incons = true; >> + } >> if (sctx->m_impure) { >> print_info(_("Directories %d missing impure xattr!\n"), >> sctx->m_impure); >> diff --git a/lib.c b/lib.c >> index 4d9536a..2aee0dc 100644 >> --- a/lib.c >> +++ b/lib.c >> @@ -244,6 +244,11 @@ int scan_dir(struct scan_ctx *sctx, struct scan_operations *sop) >> ret = scan_check_entry(sop->impurity, sctx); >> if (ret) >> goto out; >> + >> + /* Check origins */ >> + ret = scan_check_entry(sop->origin, sctx); >> + if (ret) >> + goto out; >> break; >> case FTS_DEFAULT: >> /* Check whiteouts */ >> @@ -264,6 +269,11 @@ int scan_dir(struct scan_ctx *sctx, struct scan_operations *sop) >> if (ret) >> goto out; >> >> + /* Check origins */ >> + ret = scan_check_entry(sop->origin, sctx); >> + if (ret) >> + goto out; >> + >> /* Save current dir data and create new one for subdir */ >> ftsent->fts_pointer = sctx->dirdata; >> sctx->dirdata = smalloc(sizeof(struct scan_dir_data)); >> diff --git a/lib.h b/lib.h >> index 880a8c3..5f7fdaa 100644 >> --- a/lib.h >> +++ b/lib.h >> @@ -71,7 +71,9 @@ struct scan_ctx { >> int i_whiteouts; /* invalid whiteouts */ >> int t_redirects; /* total redirect dirs */ >> int i_redirects; /* invalid redirect dirs */ >> - int m_impure; /* missing inpure dirs */ >> + int t_origins; /* total origins */ >> + int i_origins; /* invalid origins */ >> + int m_impure; /* missing impure dirs */ >> >> const char *pathname; /* path relative to overlay root */ >> const char *filename; /* filename */ >> diff --git a/overlayfs.c b/overlayfs.c >> index 93b4cad..c2f0840 100644 >> --- a/overlayfs.c >> +++ b/overlayfs.c >> @@ -19,6 +19,8 @@ >> */ >> >> #include <stddef.h> >> +#include <errno.h> >> +#include "overlayfs.h" >> >> /* >> * Split directories to individual one. >> @@ -70,3 +72,24 @@ char *ovl_next_opt(char **s) >> *s = NULL; >> return sbegin; >> } >> + >> +/* >> + * Check the validity of ovl_fh. >> + */ >> +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len) >> +{ >> + if (fh_len < sizeof(struct ovl_fh) || fh_len < fh->len) >> + return -EINVAL; >> + >> + if (fh->magic != OVL_FH_MAGIC) >> + return -EINVAL; >> + >> + if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL) >> + return -ENODATA; >> + >> + if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) && >> + (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN) >> + return -ENODATA; >> + > > Overlayfs approach to unrecognized ovl_fh on-disk format is very > naiive borderline buggy. > When endian/flags/version check fails, origin is treated as "unknown" > and that is fine as > long as the only implication is change of inode number. > But when checking that origin of upper root matches lower root for > index feature, treating > unrecognized ovl_fh format as unknown will result in overwriting the > root origin and as > far as I can tell, this is a bug. > > The situation with fsck.overlayfs is even worse IMO, because running > an old fsck tools > is even more easy to get wrong than mounting an overlayfs with an old kernel. > In general fsck.* tools, like e2fsck, are more prudent about fixing a filesystem > with unknown on-disk format than the kernel about mounting filesystem > with unknown > on-disk format features. > > This patch specifically does not seem to do any harm when encountering unknown > on-disk format of ovl_fh, but I think fsck.overlay should take some > extra steps for > safety, before fixing overlayfs with features that fsck.overlay does > not support. > > I suggest at least the following safely checks on start of fsck.overlay: > - Check if index dir exists in workdir > - Check if upperdir root has an unrecognized origin xattr format > - Check if index dir root has an unrecognized origin xattr format > > If any of the checks above is positive, fsck.overlay should issue a warning > and refuse to fix fielsystem. Running with -n -f is allowed. > See if index dir exists, then removing origin xattr can corrupt the index. > When fsck.overlay gains the knowledge about checking and fixing index, > user would opt-in to checking an overlayfs with index by -o index=on When compile the kernel, we can choose the default config about overlay, which user won't get. So, user may choose the fault arguments like "fsck.overlay -olower..." while index xattr is on when there is really overlayfs. Other filesystems can get xattr from superblock in disk, but we cannot use this way in overlay. So, how to deal with this？ -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 24, 2018 at 11:36 AM, yangerkun <yangerkun@huawei.com> wrote: > > > On 2/24/2018 3:43 PM, Amir Goldstein wrote: >> >> On Fri, Feb 23, 2018 at 2:01 PM, yangerkun <yangerkun@huawei.com> wrote: >>> >>> For origin xattr, use fid in ovl_fh to consume file_handle, and use >>> open_by_handle_at to check validity of the origin ovl_fh. In auto mode, >>> the invalid origin xattr will remove automatic. >>> >>> Signed-off-by: yangerkun <yangerkun@huawei.com> >>> --- [...] >>> + >>> +/* >>> + * Check the validity of ovl_fh. >>> + */ >>> +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len) >>> +{ >>> + if (fh_len < sizeof(struct ovl_fh) || fh_len < fh->len) >>> + return -EINVAL; >>> + >>> + if (fh->magic != OVL_FH_MAGIC) >>> + return -EINVAL; >>> + >>> + if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL) >>> + return -ENODATA; >>> + >>> + if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) && >>> + (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != >>> OVL_FH_FLAG_CPU_ENDIAN) >>> + return -ENODATA; >>> + >> >> >> Overlayfs approach to unrecognized ovl_fh on-disk format is very >> naiive borderline buggy. >> When endian/flags/version check fails, origin is treated as "unknown" >> and that is fine as >> long as the only implication is change of inode number. >> But when checking that origin of upper root matches lower root for >> index feature, treating >> unrecognized ovl_fh format as unknown will result in overwriting the >> root origin and as >> far as I can tell, this is a bug. >> >> The situation with fsck.overlayfs is even worse IMO, because running >> an old fsck tools >> is even more easy to get wrong than mounting an overlayfs with an old >> kernel. >> In general fsck.* tools, like e2fsck, are more prudent about fixing a >> filesystem >> with unknown on-disk format than the kernel about mounting filesystem >> with unknown >> on-disk format features. >> >> This patch specifically does not seem to do any harm when encountering >> unknown >> on-disk format of ovl_fh, but I think fsck.overlay should take some >> extra steps for >> safety, before fixing overlayfs with features that fsck.overlay does >> not support. >> >> I suggest at least the following safely checks on start of fsck.overlay: >> - Check if index dir exists in workdir >> - Check if upperdir root has an unrecognized origin xattr format >> - Check if index dir root has an unrecognized origin xattr format >> >> If any of the checks above is positive, fsck.overlay should issue a >> warning >> and refuse to fix fielsystem. Running with -n -f is allowed. >> See if index dir exists, then removing origin xattr can corrupt the index. >> When fsck.overlay gains the knowledge about checking and fixing index, >> user would opt-in to checking an overlayfs with index by -o index=on > > > When compile the kernel, we can choose the default config about overlay, > which user won't get. So, user may choose the fault arguments like > "fsck.overlay -olower..." while index xattr is on when there is really > overlayfs. Other filesystems can get xattr from superblock in disk, but we > cannot use this way in overlay. So, how to deal with this？ > > The kernel default options for overlayfs mount really does not matter for fsck.overlay. The only thing that matters is to detect whether the layers were *ever* mounted by a kernel which enabled features that fsck.overlay does not support. The current state of fsck.overlay is: - supports redirect feature - does not support any other feature including upstream features index and nfs_export and future features like metacopy. A filesystem repair tool should NOT fix the filesystem if it detects on-disk format that it does not recognize. That means among other things for overlayfs that if an xattr if found on any file by the name of trusted.overlay.<something> and <something> is not a an expected name, then fsck.overlay should shout that it does not recognize the on-disk format (print out the unrecognized xattr name) and refuse to continue. The problem with overlayfs compares to traditional filesystems is the lack of "feature bits" in the "super block", so there is no O(1) test for unsupported features, but they still can be detected. Contrary to the example above, there is a simple O(1) test to check if overlayfs was mounted with index feature enabled - the existence of a non empty $workdir/index dir is de-facto a "feature bit" and until fsck.overlay knows how to verify and fix overlayfs without corrupting the index it should refuse to fix an overlay filesystem with an index. The options that will be passed to fsck.overlay (i.e. redirect_dir=off, index-on) will not mean that fsck.overlay should or should not check redirects/index. They will mean something like: index=on - reconstruct index for copied up hardlinks redirect_dir=off - remove all redirects and recursively "move" all copied up files. Thanks, Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018/2/25 2:46, Amir Goldstein wrote: > On Sat, Feb 24, 2018 at 11:36 AM, yangerkun <yangerkun@huawei.com> wrote: >> >> >> On 2/24/2018 3:43 PM, Amir Goldstein wrote: >>> >>> On Fri, Feb 23, 2018 at 2:01 PM, yangerkun <yangerkun@huawei.com> wrote: >>>> >>>> For origin xattr, use fid in ovl_fh to consume file_handle, and use >>>> open_by_handle_at to check validity of the origin ovl_fh. In auto mode, >>>> the invalid origin xattr will remove automatic. >>>> >>>> Signed-off-by: yangerkun <yangerkun@huawei.com> >>>> --- > [...] >>>> + >>>> +/* >>>> + * Check the validity of ovl_fh. >>>> + */ >>>> +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len) >>>> +{ >>>> + if (fh_len < sizeof(struct ovl_fh) || fh_len < fh->len) >>>> + return -EINVAL; >>>> + >>>> + if (fh->magic != OVL_FH_MAGIC) >>>> + return -EINVAL; >>>> + >>>> + if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL) >>>> + return -ENODATA; >>>> + >>>> + if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) && >>>> + (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != >>>> OVL_FH_FLAG_CPU_ENDIAN) >>>> + return -ENODATA; >>>> + >>> >>> >>> Overlayfs approach to unrecognized ovl_fh on-disk format is very >>> naiive borderline buggy. >>> When endian/flags/version check fails, origin is treated as "unknown" >>> and that is fine as >>> long as the only implication is change of inode number. >>> But when checking that origin of upper root matches lower root for >>> index feature, treating >>> unrecognized ovl_fh format as unknown will result in overwriting the >>> root origin and as >>> far as I can tell, this is a bug. >>> >>> The situation with fsck.overlayfs is even worse IMO, because running >>> an old fsck tools >>> is even more easy to get wrong than mounting an overlayfs with an old >>> kernel. >>> In general fsck.* tools, like e2fsck, are more prudent about fixing a >>> filesystem >>> with unknown on-disk format than the kernel about mounting filesystem >>> with unknown >>> on-disk format features. >>> >>> This patch specifically does not seem to do any harm when encountering >>> unknown >>> on-disk format of ovl_fh, but I think fsck.overlay should take some >>> extra steps for >>> safety, before fixing overlayfs with features that fsck.overlay does >>> not support. >>> >>> I suggest at least the following safely checks on start of fsck.overlay: >>> - Check if index dir exists in workdir >>> - Check if upperdir root has an unrecognized origin xattr format >>> - Check if index dir root has an unrecognized origin xattr format >>> >>> If any of the checks above is positive, fsck.overlay should issue a >>> warning >>> and refuse to fix fielsystem. Running with -n -f is allowed. >>> See if index dir exists, then removing origin xattr can corrupt the index. >>> When fsck.overlay gains the knowledge about checking and fixing index, >>> user would opt-in to checking an overlayfs with index by -o index=on >> >> >> When compile the kernel, we can choose the default config about overlay, >> which user won't get. So, user may choose the fault arguments like >> "fsck.overlay -olower..." while index xattr is on when there is really >> overlayfs. Other filesystems can get xattr from superblock in disk, but we >> cannot use this way in overlay. So, how to deal with this？ >> >> > > The kernel default options for overlayfs mount really does not matter for > fsck.overlay. The only thing that matters is to detect whether the layers > were *ever* mounted by a kernel which enabled features that fsck.overlay > does not support. > > The current state of fsck.overlay is: > - supports redirect feature > - does not support any other feature including upstream features index and > nfs_export and future features like metacopy. > > A filesystem repair tool should NOT fix the filesystem if it detects on-disk > format that it does not recognize. > > That means among other things for overlayfs that if an xattr if found on any > file by the name of trusted.overlay.<something> and <something> is not a > an expected name, then fsck.overlay should shout that it does not > recognize the on-disk format (print out the unrecognized xattr name) and > refuse to continue. > > The problem with overlayfs compares to traditional filesystems is the lack > of "feature bits" in the "super block", so there is no O(1) test for unsupported > features, but they still can be detected. > > Contrary to the example above, there is a simple O(1) test to check if > overlayfs was mounted with index feature enabled - the existence of a > non empty $workdir/index dir is de-facto a "feature bit" and until fsck.overlay > knows how to verify and fix overlayfs without corrupting the index it should > refuse to fix an overlay filesystem with an index. > Maybe we can store "feature bit" in upper root xattr or somewhere else in workdir after we implement mkfs.overlay, and classify them into compat and incompat features, we can use old fsck.overlay to fix the filesystem if there is no unrecognized incompat feature. Before this, the index dir can be treated as a kind of "unrecognized incompat feature", so the "simple O(1) test" can works well, but how can we handle another incompat feature tomorrow? Should we implement mkfs.overlay before the first release version of fsck.overlay? Thanks, Yi. > The options that will be passed to fsck.overlay (i.e. > redirect_dir=off, index-on) > will not mean that fsck.overlay should or should not check redirects/index. > They will mean something like: > index=on - reconstruct index for copied up hardlinks > redirect_dir=off - remove all redirects and recursively "move" all > copied up files. > > Thanks, > Amir. > > . > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 26, 2018 at 4:10 AM, zhangyi (F) <yi.zhang@huawei.com> wrote: > On 2018/2/25 2:46, Amir Goldstein wrote: >> On Sat, Feb 24, 2018 at 11:36 AM, yangerkun <yangerkun@huawei.com> wrote: [...] >>>> I suggest at least the following safely checks on start of fsck.overlay: >>>> - Check if index dir exists in workdir >>>> - Check if upperdir root has an unrecognized origin xattr format >>>> - Check if index dir root has an unrecognized origin xattr format >>>> >>>> If any of the checks above is positive, fsck.overlay should issue a >>>> warning >>>> and refuse to fix fielsystem. Running with -n -f is allowed. >>>> See if index dir exists, then removing origin xattr can corrupt the index. >>>> When fsck.overlay gains the knowledge about checking and fixing index, >>>> user would opt-in to checking an overlayfs with index by -o index=on >>> >>> >>> When compile the kernel, we can choose the default config about overlay, >>> which user won't get. So, user may choose the fault arguments like >>> "fsck.overlay -olower..." while index xattr is on when there is really >>> overlayfs. Other filesystems can get xattr from superblock in disk, but we >>> cannot use this way in overlay. So, how to deal with this？ >>> >>> >> >> The kernel default options for overlayfs mount really does not matter for >> fsck.overlay. The only thing that matters is to detect whether the layers >> were *ever* mounted by a kernel which enabled features that fsck.overlay >> does not support. >> >> The current state of fsck.overlay is: >> - supports redirect feature >> - does not support any other feature including upstream features index and >> nfs_export and future features like metacopy. >> >> A filesystem repair tool should NOT fix the filesystem if it detects on-disk >> format that it does not recognize. >> >> That means among other things for overlayfs that if an xattr if found on any >> file by the name of trusted.overlay.<something> and <something> is not a >> an expected name, then fsck.overlay should shout that it does not >> recognize the on-disk format (print out the unrecognized xattr name) and >> refuse to continue. >> >> The problem with overlayfs compares to traditional filesystems is the lack >> of "feature bits" in the "super block", so there is no O(1) test for unsupported >> features, but they still can be detected. >> >> Contrary to the example above, there is a simple O(1) test to check if >> overlayfs was mounted with index feature enabled - the existence of a >> non empty $workdir/index dir is de-facto a "feature bit" and until fsck.overlay >> knows how to verify and fix overlayfs without corrupting the index it should >> refuse to fix an overlay filesystem with an index. >> > > Maybe we can store "feature bit" in upper root xattr or somewhere else in workdir > after we implement mkfs.overlay, and classify them into compat and incompat > features, we can use old fsck.overlay to fix the filesystem if there is no > unrecognized incompat feature. > Yes, we could and probably should. A "feature set" as xattr was proposed by Miklos when introducing redirect_dir [1]. and then I proposed a different implementation of "feature set" when introducing index feature [2]. The biggest problem with these implementation is that old kernel doesn't know about the incompat features. For this problem, fsck.overlay is at least a partial solution, because it will be possible for user to run fsck.overlay to "sanitize" an overlayfs for mount with old kernel, for example: fsck.overlay -o redirect_dir=off,index=off (*) could be used to sanitize overlayfs for mount on kernel < v4.10. (*) Another option is to follow mkfs.ext4/tune2fs standard of -O ^redirect_dir,^index. IMO, now that fsck.overlayfs has materialized, thanks to you, it is a good time to set the feature set format in stone and implement it in kernel and fsck (hint hint, if you post I will review...) > Before this, the index dir can be treated as a kind of "unrecognized incompat > feature", so the "simple O(1) test" can works well, but how can we handle > another incompat feature tomorrow? Should we implement mkfs.overlay before the > first release version of fsck.overlay? > I wouldn't say that we need to implement mkfs.overlay, but we *do* need to define the feature set on-disk format and fsck.overlay *does* have to check the incomapt features from the very first release and new kernel *should* check and set the "feature bits". I am in favor of something similar to [1], but with 3 sets of features: compat,incompat,rocompat. If I am not missing anything, upstream overlayfs now supports 4 on-disk format features that were not supported in the first upstream release (v3.18): incomapt: redirect_dir rocompat: index,nfs_export compat: constant_ino (i.e. origin+impure xattr) redirect_dir can be detected only by scanning all dirs. constant_ino can be detected only by scanning all files. It's not really that important to cleanup a compat feature that the first version of fsck supports, but its not hard to implement it either. index can be detected by checking index dir exists (or not empty). nfs_export can be detected by checking if index dir contains whiteout and directory entries. IMO we should not merge another incompat feature like metacopy before defining the feature set format and setting the incompat "feature bit" on first mount with metacopy enabled. Thanks, Amir. [1] https://marc.info/?l=linux-unionfs&m=147739471816673&w=2 [2] https://marc.info/?l=linux-unionfs&m=150885903123037&w=2 -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/check.c b/check.c index e109620..50c4585 100644 --- a/check.c +++ b/check.c @@ -24,6 +24,7 @@ #include <stdlib.h> #include <stdio.h> +#include <stddef.h> #include <string.h> #include <errno.h> #include <unistd.h> @@ -169,11 +170,42 @@ static int ovl_get_redirect(int dirfd, const char *pathname, return 0; } +/* + * Get the origin file handle with the pathname, then check the validity of member + * in ovl_fh. + */ +static int ovl_get_fh(int dirfd, const char *pathname, + struct ovl_fh **fh, bool *invalid) +{ + char *og = NULL; + ssize_t ret; + + ret = get_xattr(dirfd, pathname, OVL_ORIGIN_XATTR, &og, NULL); + if (ret <= 0) + return ret; + + ret = ovl_check_fh_len((struct ovl_fh *)og, ret); + if (ret) { + free(og); + if (ret == -EINVAL) + *invalid = true; + } else { + *fh = (struct ovl_fh *)og; + } + + return 0; +} + static inline int ovl_remove_redirect(int dirfd, const char *pathname) { return remove_xattr(dirfd, pathname, OVL_REDIRECT_XATTR); } +static inline int ovl_remove_origin(int dirfd, const char *pathname) +{ + return remove_xattr(dirfd, pathname, OVL_ORIGIN_XATTR); +} + static inline int ovl_create_whiteout(int dirfd, const char *pathname) { if (mknodat(dirfd, pathname, S_IFCHR | WHITEOUT_MOD, makedev(0, 0))) { @@ -758,6 +790,97 @@ out: } /* + * Get origin file handle stored in the xattr, check it's invalid + * or not. In auto-mode, invalid origin xattr will be removed directly. + */ +static int ovl_check_origin(struct scan_ctx *sctx) +{ + struct ovl_fh *fh = NULL; + struct file_handle *fhp = NULL; + struct stat stat; + bool invalid = false; + int i; + int ret; + int fid_len; + int fd; + + /* Get origin */ + ret = ovl_get_fh(sctx->dirfd, sctx->pathname, &fh, &invalid); + if (ret < 0) + return -1; + + /* Cannot get ovl_fh, maybe not set or unrecognized */ + if (!fh && !invalid) + return 0; + + print_debug(_("File or dir \"%s\" has origin\n"), sctx->pathname); + sctx->t_origins++; + + /* ovl_fh is invalid */ + if (invalid) + goto remove; + + /* Origin file in last lower dir? */ + if (sctx->dirtype == OVL_LOWER && sctx->stack == lower_num -1) + goto remove; + + fid_len = fh->len - offsetof(struct ovl_fh, fid); + fhp = smalloc(sizeof(struct file_handle) + fid_len); + fhp->handle_bytes = fh->len - offsetof(struct ovl_fh, fid); + fhp->handle_type = fh->type; + memcpy(fhp->f_handle, fh->fid, fid_len); + + /* Scan lower directories to check origin xattr's validity */ + i = sctx->dirtype == OVL_UPPER ? 0 : sctx->stack + 1; + for (; i < lower_num; i++) { + fd = open_by_handle_at(lowerfd[i], fhp, O_RDONLY); + + if (fd == -1) { + if (errno == ESTALE || errno == ENOENT) + continue; + + print_err(_("Error open_by_handle_at %s:%s\n"), + sctx->pathname, strerror(errno)); + ret = -1; + goto out; + } + + ret = fstat(fd, &stat); + if (ret) { + print_err(_("Cannot fstat origin file in %s of %s:%s\n"), + lowerdir[i], sctx->pathname, strerror(errno)); + goto out2; + } + + if (stat.st_mode != sctx->st->st_mode) + goto remove; + + goto out2; + } + +remove: + sctx->i_origins++; + + /* Remove origin xattr or ask user */ + if (!ovl_ask_action("Invalid origin xattr", sctx->pathname, sctx->dirtype, + sctx->stack, "Remove origin", 1)) + goto out; + + ret = ovl_remove_origin(sctx->dirfd, sctx->pathname); + if (ret) + goto out; + + sctx->i_origins--; + sctx->t_origins--; +out2: + close(fd); +out: + free(fhp); + free(fh); + return ret; +} + +/* * If a directory has origin target and redirect/merge subdirectories in it, * it may contain copied up targets. In order to avoid 'd_ino' change after * lower target copy-up or rename (which will create a new inode), @@ -870,9 +993,11 @@ static struct scan_operations ovl_scan_ops[OVL_SCAN_PASS][2] = { .whiteout = ovl_check_whiteout, .impurity = ovl_count_impurity, .impure = ovl_check_impure, + .origin = ovl_check_origin, }, [OVL_LOWER] = { .whiteout = ovl_check_whiteout, + .origin = ovl_check_origin, }, }/* Pass Two */ }; @@ -892,11 +1017,12 @@ static void ovl_scan_report(struct scan_ctx *sctx) { if (flags & FL_VERBOSE) { print_info(_("Scan %d directories, %d files, " - "%d/%d whiteouts, %d/%d redirect dirs " - "%d missing impure\n"), + "%d/%d whiteouts, %d/%d redirect dirs, " + "%d/%d origins, %d missing impure\n"), sctx->directories, sctx->files, sctx->i_whiteouts, sctx->t_whiteouts, sctx->i_redirects, sctx->t_redirects, + sctx->i_origins, sctx->t_origins, sctx->m_impure); } } @@ -916,6 +1042,11 @@ static void ovl_scan_check(struct scan_ctx *sctx) sctx->i_redirects); incons = true; } + if (sctx->i_origins) { + print_info(_("Invalid origins %d left!\n"), + sctx->i_origins); + incons = true; + } if (sctx->m_impure) { print_info(_("Directories %d missing impure xattr!\n"), sctx->m_impure); diff --git a/lib.c b/lib.c index 4d9536a..2aee0dc 100644 --- a/lib.c +++ b/lib.c @@ -244,6 +244,11 @@ int scan_dir(struct scan_ctx *sctx, struct scan_operations *sop) ret = scan_check_entry(sop->impurity, sctx); if (ret) goto out; + + /* Check origins */ + ret = scan_check_entry(sop->origin, sctx); + if (ret) + goto out; break; case FTS_DEFAULT: /* Check whiteouts */ @@ -264,6 +269,11 @@ int scan_dir(struct scan_ctx *sctx, struct scan_operations *sop) if (ret) goto out; + /* Check origins */ + ret = scan_check_entry(sop->origin, sctx); + if (ret) + goto out; + /* Save current dir data and create new one for subdir */ ftsent->fts_pointer = sctx->dirdata; sctx->dirdata = smalloc(sizeof(struct scan_dir_data)); diff --git a/lib.h b/lib.h index 880a8c3..5f7fdaa 100644 --- a/lib.h +++ b/lib.h @@ -71,7 +71,9 @@ struct scan_ctx { int i_whiteouts; /* invalid whiteouts */ int t_redirects; /* total redirect dirs */ int i_redirects; /* invalid redirect dirs */ - int m_impure; /* missing inpure dirs */ + int t_origins; /* total origins */ + int i_origins; /* invalid origins */ + int m_impure; /* missing impure dirs */ const char *pathname; /* path relative to overlay root */ const char *filename; /* filename */ diff --git a/overlayfs.c b/overlayfs.c index 93b4cad..c2f0840 100644 --- a/overlayfs.c +++ b/overlayfs.c @@ -19,6 +19,8 @@ */ #include <stddef.h> +#include <errno.h> +#include "overlayfs.h" /* * Split directories to individual one. @@ -70,3 +72,24 @@ char *ovl_next_opt(char **s) *s = NULL; return sbegin; } + +/* + * Check the validity of ovl_fh. + */ +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len) +{ + if (fh_len < sizeof(struct ovl_fh) || fh_len < fh->len) + return -EINVAL; + + if (fh->magic != OVL_FH_MAGIC) + return -EINVAL; + + if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL) + return -ENODATA; + + if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) && + (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN) + return -ENODATA; + + return 0; +} diff --git a/overlayfs.h b/overlayfs.h index e3798aa..9913912 100644 --- a/overlayfs.h +++ b/overlayfs.h @@ -42,7 +42,40 @@ #define OVL_ORIGIN_XATTR "trusted.overlay.origin" #define OVL_IMPURE_XATTR "trusted.overlay.impure" +#define OVL_FH_VERSION 0 +#define OVL_FH_MAGIC 0xfb + +#define OVL_FH_FLAG_BIG_ENDIAN (1 << 0) +#define OVL_FH_FLAG_ANY_ENDIAN (1 << 1) +#define OVL_FH_FLAG_PATH_UPPER (1 << 2) +#define OVL_FH_FLAG_ALL (OVL_FH_FLAG_BIG_ENDIAN | OVL_FH_FLAG_ANY_ENDIAN | \ + OVL_FH_FLAG_PATH_UPPER) +#include <endian.h> +#if __BYTE_ORDER == __LITTLE_ENDIAN +#define OVL_FH_FLAG_CPU_ENDIAN 0 +#elif __BYTE_ORDER == __BIG_ENDIAN +#define OVL_FH_FLAG_CPU_ENDIAN OVL_FH_FLAG_BIG_ENDIAN +#else +#error Endianness not defined +#endif + +#include <stdint.h> +typedef struct { + uint8_t b[16]; +}uuid_t; + +struct ovl_fh { + uint8_t version; + uint8_t magic; + uint8_t len; + uint8_t flags; + uint8_t type; + uuid_t uuid; + uint8_t fid[0]; +}; + unsigned int ovl_split_lowerdirs(char *lower); char *ovl_next_opt(char **s); +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len); #endif /* OVL_OVERLAYFS_H */
For origin xattr, use fid in ovl_fh to consume file_handle, and use open_by_handle_at to check validity of the origin ovl_fh. In auto mode, the invalid origin xattr will remove automatic. Signed-off-by: yangerkun <yangerkun@huawei.com> --- check.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- lib.c | 10 +++++ lib.h | 4 +- overlayfs.c | 23 +++++++++++ overlayfs.h | 33 +++++++++++++++ 5 files changed, 202 insertions(+), 3 deletions(-)