| Message ID | 1515086449-26563-4-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[Added Al Viro] On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: > Decoding an upper file handle is done by decoding the upper dentry from > underlying upper fs, finding or allocating an overlay inode that is > hashed by the real upper inode and instantiating an overlay dentry with > that inode. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ > fs/overlayfs/namei.c | 4 +-- > fs/overlayfs/overlayfs.h | 2 ++ > 3 files changed, 95 insertions(+), 2 deletions(-) > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c > index 58c4f5e8a67e..5c72784a0b4d 100644 > --- a/fs/overlayfs/export.c > +++ b/fs/overlayfs/export.c > @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, > return type; > } > > +/* > + * Find or instantiate an overlay dentry from real dentries. > + */ > +static struct dentry *ovl_obtain_alias(struct super_block *sb, > + struct dentry *upper, > + struct ovl_path *lowerpath) > +{ > + struct inode *inode; > + struct dentry *dentry; > + struct ovl_entry *oe; > + > + /* TODO: obtain non pure-upper */ > + if (lowerpath) > + return ERR_PTR(-EIO); > + > + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); > + if (IS_ERR(inode)) { > + dput(upper); > + return ERR_CAST(inode); > + } > + > + dentry = d_obtain_alias(inode); > + if (IS_ERR(dentry) || dentry->d_fsdata) Racing two instances of this code, each thinking it got a new alias and trying to fill it, results in a memory leak. Haven't checked in too much depth, but apparently other filesystems are not affected, so we need something special here. One solution: split d_instantiate_anon(dentry, inode) out of __d_obtain_alias() and supply that with the already initialized dentry. Al? Thanks, Miklos > + return dentry; > + > + oe = ovl_alloc_entry(0); > + if (!oe) { > + dput(dentry); > + return ERR_PTR(-ENOMEM); > + } > + > + dentry->d_fsdata = oe; > + ovl_dentry_set_upper_alias(dentry); > + > + return dentry; > +} > + > +static struct dentry *ovl_upper_fh_to_d(struct super_block *sb, > + struct ovl_fh *fh) > +{ > + struct ovl_fs *ofs = sb->s_fs_info; > + struct dentry *dentry; > + struct dentry *upper; > + > + if (!ofs->upper_mnt) > + return ERR_PTR(-EACCES); > + > + upper = ovl_decode_fh(fh, ofs->upper_mnt); > + if (IS_ERR_OR_NULL(upper)) > + return upper; > + > + dentry = ovl_obtain_alias(sb, upper, NULL); > + dput(upper); > + > + return dentry; > +} > + > +static struct dentry *ovl_fh_to_dentry(struct super_block *sb, struct fid *fid, > + int fh_len, int fh_type) > +{ > + struct dentry *dentry = NULL; > + struct ovl_fh *fh = (struct ovl_fh *) fid; > + int len = fh_len << 2; > + unsigned int flags = 0; > + int err; > + > + err = -EINVAL; > + if (fh_type != OVL_FILEID) > + goto out_err; > + > + err = ovl_check_fh_len(fh, len); > + if (err) > + goto out_err; > + > + /* TODO: decode non-upper */ > + flags = fh->flags; > + if (flags & OVL_FH_FLAG_PATH_UPPER) > + dentry = ovl_upper_fh_to_d(sb, fh); > + err = PTR_ERR(dentry); > + if (IS_ERR(dentry) && err != -ESTALE) > + goto out_err; > + > + return dentry; > + > +out_err: > + pr_warn_ratelimited("overlayfs: failed to decode file handle (len=%d, type=%d, flags=%x, err=%i)\n", > + len, fh_type, flags, err); > + return ERR_PTR(err); > +} > + > const struct export_operations ovl_export_operations = { > .encode_fh = ovl_encode_inode_fh, > + .fh_to_dentry = ovl_fh_to_dentry, > }; > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index a69cedf06000..87d39384dc55 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -107,7 +107,7 @@ static int ovl_acceptable(void *ctx, struct dentry *dentry) > * Return -ENODATA for "origin unknown". > * Return <0 for an invalid file handle. > */ > -static int ovl_check_fh_len(struct ovl_fh *fh, int fh_len) > +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; > @@ -171,7 +171,7 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry) > goto out; > } > > -static struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt) > +struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt) > { > struct dentry *origin; > int bytes; > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > index f6fd999cb98e..c4f8e98e209e 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -258,6 +258,8 @@ static inline bool ovl_is_impuredir(struct dentry *dentry) > > > /* namei.c */ > +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len); > +struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt); > int ovl_verify_origin(struct dentry *dentry, struct dentry *origin, > bool is_upper, bool set); > int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index); > -- > 2.7.4 >
On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > [Added Al Viro] > > On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> Decoding an upper file handle is done by decoding the upper dentry from >> underlying upper fs, finding or allocating an overlay inode that is >> hashed by the real upper inode and instantiating an overlay dentry with >> that inode. >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ >> fs/overlayfs/namei.c | 4 +-- >> fs/overlayfs/overlayfs.h | 2 ++ >> 3 files changed, 95 insertions(+), 2 deletions(-) >> >> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c >> index 58c4f5e8a67e..5c72784a0b4d 100644 >> --- a/fs/overlayfs/export.c >> +++ b/fs/overlayfs/export.c >> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, >> return type; >> } >> >> +/* >> + * Find or instantiate an overlay dentry from real dentries. >> + */ >> +static struct dentry *ovl_obtain_alias(struct super_block *sb, >> + struct dentry *upper, >> + struct ovl_path *lowerpath) >> +{ >> + struct inode *inode; >> + struct dentry *dentry; >> + struct ovl_entry *oe; >> + >> + /* TODO: obtain non pure-upper */ >> + if (lowerpath) >> + return ERR_PTR(-EIO); >> + >> + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); >> + if (IS_ERR(inode)) { >> + dput(upper); >> + return ERR_CAST(inode); >> + } >> + >> + dentry = d_obtain_alias(inode); >> + if (IS_ERR(dentry) || dentry->d_fsdata) > > Racing two instances of this code, each thinking it got a new alias > and trying to fill it, results in a memory leak. > > Haven't checked in too much depth, but apparently other filesystems > are not affected, so we need something special here. > > One solution: split d_instantiate_anon(dentry, inode) out of > __d_obtain_alias() and supply that with the already initialized > dentry. > Can't we use &OVL_I(inode)->lock to avoid the race? Thanks, Amir.
On Thu, Jan 18, 2018 at 3:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> [Added Al Viro] >> >> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> Decoding an upper file handle is done by decoding the upper dentry from >>> underlying upper fs, finding or allocating an overlay inode that is >>> hashed by the real upper inode and instantiating an overlay dentry with >>> that inode. >>> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>> --- >>> fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ >>> fs/overlayfs/namei.c | 4 +-- >>> fs/overlayfs/overlayfs.h | 2 ++ >>> 3 files changed, 95 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c >>> index 58c4f5e8a67e..5c72784a0b4d 100644 >>> --- a/fs/overlayfs/export.c >>> +++ b/fs/overlayfs/export.c >>> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, >>> return type; >>> } >>> >>> +/* >>> + * Find or instantiate an overlay dentry from real dentries. >>> + */ >>> +static struct dentry *ovl_obtain_alias(struct super_block *sb, >>> + struct dentry *upper, >>> + struct ovl_path *lowerpath) >>> +{ >>> + struct inode *inode; >>> + struct dentry *dentry; >>> + struct ovl_entry *oe; >>> + >>> + /* TODO: obtain non pure-upper */ >>> + if (lowerpath) >>> + return ERR_PTR(-EIO); >>> + >>> + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); >>> + if (IS_ERR(inode)) { >>> + dput(upper); >>> + return ERR_CAST(inode); >>> + } >>> + >>> + dentry = d_obtain_alias(inode); >>> + if (IS_ERR(dentry) || dentry->d_fsdata) >> >> Racing two instances of this code, each thinking it got a new alias >> and trying to fill it, results in a memory leak. >> >> Haven't checked in too much depth, but apparently other filesystems >> are not affected, so we need something special here. >> >> One solution: split d_instantiate_anon(dentry, inode) out of >> __d_obtain_alias() and supply that with the already initialized >> dentry. >> > > Can't we use &OVL_I(inode)->lock to avoid the race? We could. But then d_splice_alias() will find our half baked dentry and return that from ovl_lookup(). So we do need to have the dentry fully initialized by the time it's added into the inode's alias list. Thanks, Miklos
On Thu, Jan 18, 2018 at 4:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Thu, Jan 18, 2018 at 3:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> [Added Al Viro] >>> >>> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> Decoding an upper file handle is done by decoding the upper dentry from >>>> underlying upper fs, finding or allocating an overlay inode that is >>>> hashed by the real upper inode and instantiating an overlay dentry with >>>> that inode. >>>> >>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>>> --- >>>> fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>> fs/overlayfs/namei.c | 4 +-- >>>> fs/overlayfs/overlayfs.h | 2 ++ >>>> 3 files changed, 95 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c >>>> index 58c4f5e8a67e..5c72784a0b4d 100644 >>>> --- a/fs/overlayfs/export.c >>>> +++ b/fs/overlayfs/export.c >>>> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, >>>> return type; >>>> } >>>> >>>> +/* >>>> + * Find or instantiate an overlay dentry from real dentries. >>>> + */ >>>> +static struct dentry *ovl_obtain_alias(struct super_block *sb, >>>> + struct dentry *upper, >>>> + struct ovl_path *lowerpath) >>>> +{ >>>> + struct inode *inode; >>>> + struct dentry *dentry; >>>> + struct ovl_entry *oe; >>>> + >>>> + /* TODO: obtain non pure-upper */ >>>> + if (lowerpath) >>>> + return ERR_PTR(-EIO); >>>> + >>>> + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); >>>> + if (IS_ERR(inode)) { >>>> + dput(upper); >>>> + return ERR_CAST(inode); >>>> + } >>>> + >>>> + dentry = d_obtain_alias(inode); >>>> + if (IS_ERR(dentry) || dentry->d_fsdata) >>> >>> Racing two instances of this code, each thinking it got a new alias >>> and trying to fill it, results in a memory leak. >>> >>> Haven't checked in too much depth, but apparently other filesystems >>> are not affected, so we need something special here. >>> >>> One solution: split d_instantiate_anon(dentry, inode) out of >>> __d_obtain_alias() and supply that with the already initialized >>> dentry. >>> >> >> Can't we use &OVL_I(inode)->lock to avoid the race? > > We could. But then d_splice_alias() will find our half baked dentry > and return that from ovl_lookup(). No it won't, because we do not obtain dir dentries this way. We actually do in this patch [3/17], but since patch [4/17] we don't, so I only need to fix this patch not to obtain dir dentry and to protect concurrent decode of non-dir with &OVL_I(inode)->lock. > So we do need to have the dentry > fully initialized by the time it's added into the inode's alias list. > The only problems I see with adding a non-dir disconnected alias that is not fully baked are: 1. We can get it in ovl_encode_inode_fh() from d_find_any_alias() 2. nfsd can get it in exportfs_decode_fh() from find_acceptable_alias() in a weird hypothetical case where the fully baked dentry we just returned from ovl_obtain_alias() in NOT acceptable by nfsd but the half baked dentry IS acceptable 3. Another kernel user that uses d_find_any_alias() or one of the use case that only Al can think of... Cases 2 and 3, I don't know if they are for real. Case 1 is only a problem due to lack of export_operations method 'dentry_to_fh'. exportfs_encode_fh() has the right dentry, but it does not pass it to the filesystem for encoding, so I think it should be solved by adding this method. Thanks, Amir.
On Thu, Jan 18, 2018 at 8:49 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Thu, Jan 18, 2018 at 4:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Thu, Jan 18, 2018 at 3:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>> [Added Al Viro] >>>> >>>> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>>> Decoding an upper file handle is done by decoding the upper dentry from >>>>> underlying upper fs, finding or allocating an overlay inode that is >>>>> hashed by the real upper inode and instantiating an overlay dentry with >>>>> that inode. >>>>> >>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>>>> --- >>>>> fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> fs/overlayfs/namei.c | 4 +-- >>>>> fs/overlayfs/overlayfs.h | 2 ++ >>>>> 3 files changed, 95 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c >>>>> index 58c4f5e8a67e..5c72784a0b4d 100644 >>>>> --- a/fs/overlayfs/export.c >>>>> +++ b/fs/overlayfs/export.c >>>>> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, >>>>> return type; >>>>> } >>>>> >>>>> +/* >>>>> + * Find or instantiate an overlay dentry from real dentries. >>>>> + */ >>>>> +static struct dentry *ovl_obtain_alias(struct super_block *sb, >>>>> + struct dentry *upper, >>>>> + struct ovl_path *lowerpath) >>>>> +{ >>>>> + struct inode *inode; >>>>> + struct dentry *dentry; >>>>> + struct ovl_entry *oe; >>>>> + >>>>> + /* TODO: obtain non pure-upper */ >>>>> + if (lowerpath) >>>>> + return ERR_PTR(-EIO); >>>>> + >>>>> + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); >>>>> + if (IS_ERR(inode)) { >>>>> + dput(upper); >>>>> + return ERR_CAST(inode); >>>>> + } >>>>> + >>>>> + dentry = d_obtain_alias(inode); >>>>> + if (IS_ERR(dentry) || dentry->d_fsdata) >>>> >>>> Racing two instances of this code, each thinking it got a new alias >>>> and trying to fill it, results in a memory leak. >>>> >>>> Haven't checked in too much depth, but apparently other filesystems >>>> are not affected, so we need something special here. >>>> >>>> One solution: split d_instantiate_anon(dentry, inode) out of >>>> __d_obtain_alias() and supply that with the already initialized >>>> dentry. >>>> >>> >>> Can't we use &OVL_I(inode)->lock to avoid the race? >> >> We could. But then d_splice_alias() will find our half baked dentry >> and return that from ovl_lookup(). > > No it won't, because we do not obtain dir dentries this way. > We actually do in this patch [3/17], but since patch [4/17] we don't, > so I only need to fix this patch not to obtain dir dentry and to > protect concurrent decode of non-dir with &OVL_I(inode)->lock. > >> So we do need to have the dentry >> fully initialized by the time it's added into the inode's alias list. >> > > The only problems I see with adding a non-dir disconnected alias > that is not fully baked are: > 1. We can get it in ovl_encode_inode_fh() from d_find_any_alias() > 2. nfsd can get it in exportfs_decode_fh() from find_acceptable_alias() > in a weird hypothetical case where the fully baked dentry we just > returned from ovl_obtain_alias() in NOT acceptable by nfsd but > the half baked dentry IS acceptable > 3. Another kernel user that uses d_find_any_alias() or one of the use > case that only Al can think of... > > Cases 2 and 3, I don't know if they are for real. > > Case 1 is only a problem due to lack of export_operations method > 'dentry_to_fh'. exportfs_encode_fh() has the right dentry, but it does > not pass it to the filesystem for encoding, so I think it should be > solved by adding this method. I agree with your analysis. However, I don't see what's wrong with adding fully baked dentries to the inode. To me having the dentry in a consistent state when it's linked to the inode looks far safer and easier than trying to work around inconsistent dentries by creating new interfaces. Thanks, Miklos
On Thu, Jan 18, 2018 at 10:10 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Thu, Jan 18, 2018 at 8:49 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Thu, Jan 18, 2018 at 4:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Thu, Jan 18, 2018 at 3:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>>> [Added Al Viro] >>>>> >>>>> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>>>> Decoding an upper file handle is done by decoding the upper dentry from >>>>>> underlying upper fs, finding or allocating an overlay inode that is >>>>>> hashed by the real upper inode and instantiating an overlay dentry with >>>>>> that inode. >>>>>> >>>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>>>>> --- >>>>>> fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> fs/overlayfs/namei.c | 4 +-- >>>>>> fs/overlayfs/overlayfs.h | 2 ++ >>>>>> 3 files changed, 95 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c >>>>>> index 58c4f5e8a67e..5c72784a0b4d 100644 >>>>>> --- a/fs/overlayfs/export.c >>>>>> +++ b/fs/overlayfs/export.c >>>>>> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, >>>>>> return type; >>>>>> } >>>>>> >>>>>> +/* >>>>>> + * Find or instantiate an overlay dentry from real dentries. >>>>>> + */ >>>>>> +static struct dentry *ovl_obtain_alias(struct super_block *sb, >>>>>> + struct dentry *upper, >>>>>> + struct ovl_path *lowerpath) >>>>>> +{ >>>>>> + struct inode *inode; >>>>>> + struct dentry *dentry; >>>>>> + struct ovl_entry *oe; >>>>>> + >>>>>> + /* TODO: obtain non pure-upper */ >>>>>> + if (lowerpath) >>>>>> + return ERR_PTR(-EIO); >>>>>> + >>>>>> + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); >>>>>> + if (IS_ERR(inode)) { >>>>>> + dput(upper); >>>>>> + return ERR_CAST(inode); >>>>>> + } >>>>>> + >>>>>> + dentry = d_obtain_alias(inode); >>>>>> + if (IS_ERR(dentry) || dentry->d_fsdata) >>>>> >>>>> Racing two instances of this code, each thinking it got a new alias >>>>> and trying to fill it, results in a memory leak. >>>>> >>>>> Haven't checked in too much depth, but apparently other filesystems >>>>> are not affected, so we need something special here. >>>>> >>>>> One solution: split d_instantiate_anon(dentry, inode) out of >>>>> __d_obtain_alias() and supply that with the already initialized >>>>> dentry. >>>>> >>>> >>>> Can't we use &OVL_I(inode)->lock to avoid the race? >>> >>> We could. But then d_splice_alias() will find our half baked dentry >>> and return that from ovl_lookup(). >> >> No it won't, because we do not obtain dir dentries this way. >> We actually do in this patch [3/17], but since patch [4/17] we don't, >> so I only need to fix this patch not to obtain dir dentry and to >> protect concurrent decode of non-dir with &OVL_I(inode)->lock. >> >>> So we do need to have the dentry >>> fully initialized by the time it's added into the inode's alias list. >>> >> >> The only problems I see with adding a non-dir disconnected alias >> that is not fully baked are: >> 1. We can get it in ovl_encode_inode_fh() from d_find_any_alias() >> 2. nfsd can get it in exportfs_decode_fh() from find_acceptable_alias() >> in a weird hypothetical case where the fully baked dentry we just >> returned from ovl_obtain_alias() in NOT acceptable by nfsd but >> the half baked dentry IS acceptable >> 3. Another kernel user that uses d_find_any_alias() or one of the use >> case that only Al can think of... >> >> Cases 2 and 3, I don't know if they are for real. >> >> Case 1 is only a problem due to lack of export_operations method >> 'dentry_to_fh'. exportfs_encode_fh() has the right dentry, but it does >> not pass it to the filesystem for encoding, so I think it should be >> solved by adding this method. > > I agree with your analysis. > > However, I don't see what's wrong with adding fully baked dentries to > the inode. I agree that adding half baked dentries is not a good practice and we should avoid it. > To me having the dentry in a consistent state when it's > linked to the inode looks far safer and easier than trying to work > around inconsistent dentries by creating new interfaces. > Actually, this interface is something I wanted to add to begin with. I think the current implementation that uses d_find_any_alias() is probably sub-optimal. When I tried to implement connectable (non-dir) file handles, I had to write a special helper to find an alias of inode whose parent dir is 'parent'. All this instead of passing dentry on the interface? and why? because traditionally file systems only needed inode and parent inode to encode? It's not breaking any abstraction layer to pass in dentry, in fact that is the API that nfsd uses exportfs_encode_fh(dentry, fh...), why not pass the same API to filesystem. I am going to post this new interface, because I think it is the right interface to use. I will take care of the memory leak and add will leave it to you and Al to come up with the solution for half baked dentries. When you agree on a solution I can implement it. Thanks, Amir.
On Thu, Jan 18, 2018 at 10:35 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Thu, Jan 18, 2018 at 10:10 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Thu, Jan 18, 2018 at 8:49 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> On Thu, Jan 18, 2018 at 4:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>> On Thu, Jan 18, 2018 at 3:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>>> On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>>>> [Added Al Viro] >>>>>> >>>>>> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>>>>> Decoding an upper file handle is done by decoding the upper dentry from >>>>>>> underlying upper fs, finding or allocating an overlay inode that is >>>>>>> hashed by the real upper inode and instantiating an overlay dentry with >>>>>>> that inode. >>>>>>> >>>>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>>>>>> --- >>>>>>> fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>> fs/overlayfs/namei.c | 4 +-- >>>>>>> fs/overlayfs/overlayfs.h | 2 ++ >>>>>>> 3 files changed, 95 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c >>>>>>> index 58c4f5e8a67e..5c72784a0b4d 100644 >>>>>>> --- a/fs/overlayfs/export.c >>>>>>> +++ b/fs/overlayfs/export.c >>>>>>> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, >>>>>>> return type; >>>>>>> } >>>>>>> >>>>>>> +/* >>>>>>> + * Find or instantiate an overlay dentry from real dentries. >>>>>>> + */ >>>>>>> +static struct dentry *ovl_obtain_alias(struct super_block *sb, >>>>>>> + struct dentry *upper, >>>>>>> + struct ovl_path *lowerpath) >>>>>>> +{ >>>>>>> + struct inode *inode; >>>>>>> + struct dentry *dentry; >>>>>>> + struct ovl_entry *oe; >>>>>>> + >>>>>>> + /* TODO: obtain non pure-upper */ >>>>>>> + if (lowerpath) >>>>>>> + return ERR_PTR(-EIO); >>>>>>> + >>>>>>> + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); >>>>>>> + if (IS_ERR(inode)) { >>>>>>> + dput(upper); >>>>>>> + return ERR_CAST(inode); >>>>>>> + } >>>>>>> + >>>>>>> + dentry = d_obtain_alias(inode); >>>>>>> + if (IS_ERR(dentry) || dentry->d_fsdata) >>>>>> >>>>>> Racing two instances of this code, each thinking it got a new alias >>>>>> and trying to fill it, results in a memory leak. >>>>>> >>>>>> Haven't checked in too much depth, but apparently other filesystems >>>>>> are not affected, so we need something special here. >>>>>> >>>>>> One solution: split d_instantiate_anon(dentry, inode) out of >>>>>> __d_obtain_alias() and supply that with the already initialized >>>>>> dentry. >>>>>> >>>>> >>>>> Can't we use &OVL_I(inode)->lock to avoid the race? >>>> >>>> We could. But then d_splice_alias() will find our half baked dentry >>>> and return that from ovl_lookup(). >>> >>> No it won't, because we do not obtain dir dentries this way. >>> We actually do in this patch [3/17], but since patch [4/17] we don't, >>> so I only need to fix this patch not to obtain dir dentry and to >>> protect concurrent decode of non-dir with &OVL_I(inode)->lock. >>> >>>> So we do need to have the dentry >>>> fully initialized by the time it's added into the inode's alias list. >>>> >>> >>> The only problems I see with adding a non-dir disconnected alias >>> that is not fully baked are: >>> 1. We can get it in ovl_encode_inode_fh() from d_find_any_alias() >>> 2. nfsd can get it in exportfs_decode_fh() from find_acceptable_alias() >>> in a weird hypothetical case where the fully baked dentry we just >>> returned from ovl_obtain_alias() in NOT acceptable by nfsd but >>> the half baked dentry IS acceptable >>> 3. Another kernel user that uses d_find_any_alias() or one of the use >>> case that only Al can think of... >>> >>> Cases 2 and 3, I don't know if they are for real. >>> >>> Case 1 is only a problem due to lack of export_operations method >>> 'dentry_to_fh'. exportfs_encode_fh() has the right dentry, but it does >>> not pass it to the filesystem for encoding, so I think it should be >>> solved by adding this method. >> >> I agree with your analysis. >> >> However, I don't see what's wrong with adding fully baked dentries to >> the inode. > > I agree that adding half baked dentries is not a good practice and > we should avoid it. > How is this for an option? =========================================== +/* + * Find or instantiate an overlay dentry from real dentries. + */ +static struct dentry *ovl_obtain_alias(struct super_block *sb, + struct dentry *upper, + struct ovl_path *lowerpath) +{ + struct inode *inode; + struct dentry *dentry; + struct ovl_entry *oe; + void *fsdata = &oe; + + /* TODO: obtain non pure-upper */ + if (lowerpath) + return ERR_PTR(-EIO); + + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); + if (IS_ERR(inode)) { + dput(upper); + return ERR_CAST(inode); + } + + oe = ovl_alloc_entry(0); + if (!oe) { + iput(inode); + return ERR_PTR(-ENOMEM); + } + oe->has_upper = true; + + dentry = d_obtain_alias_fsdata(inode, fsdata); + /* A new allocated dentry assigns *fsdata and sets it to NULL */ + if (oe) + kfree(oe); + + return dentry; +} + ... -static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) +static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected, + void **fsdata) { struct dentry *tmp; struct dentry *res; @@ -1962,6 +1963,12 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) if (disconnected) add_flags |= DCACHE_DISCONNECTED; + /* Take ownership of pre-allocated fs-specific data */ + if (fsdata) { + tmp->d_fsdata = fsdata; + *fsdata = NULL; + } + spin_lock(&tmp->d_lock); __d_set_inode_and_type(tmp, inode, add_flags); hlist_add_head(&tmp->d_u.d_alias, &inode->i_dentry); @@ -1998,7 +2005,13 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) */ struct dentry *d_obtain_alias(struct inode *inode) { - return __d_obtain_alias(inode, 1); + return __d_obtain_alias(inode, 1, NULL); +} +EXPORT_SYMBOL(d_obtain_alias); + +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) +{ + return __d_obtain_alias(inode, 1, fsdata); } EXPORT_SYMBOL(d_obtain_alias); @@ -2019,7 +2032,7 @@ EXPORT_SYMBOL(d_obtain_alias); */ struct dentry *d_obtain_root(struct inode *inode) { - return __d_obtain_alias(inode, 0); + return __d_obtain_alias(inode, 0, NULL); } EXPORT_SYMBOL(d_obtain_root);
On Fri, Jan 19, 2018 at 12:57 AM, Amir Goldstein <amir73il@gmail.com> wrote: > On Thu, Jan 18, 2018 at 10:35 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Thu, Jan 18, 2018 at 10:10 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Thu, Jan 18, 2018 at 8:49 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> On Thu, Jan 18, 2018 at 4:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>>> On Thu, Jan 18, 2018 at 3:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>>>> On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>>>>> [Added Al Viro] >>>>>>> >>>>>>> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>>>>>> Decoding an upper file handle is done by decoding the upper dentry from >>>>>>>> underlying upper fs, finding or allocating an overlay inode that is >>>>>>>> hashed by the real upper inode and instantiating an overlay dentry with >>>>>>>> that inode. >>>>>>>> >>>>>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>>>>>>> --- >>>>>>>> fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> fs/overlayfs/namei.c | 4 +-- >>>>>>>> fs/overlayfs/overlayfs.h | 2 ++ >>>>>>>> 3 files changed, 95 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c >>>>>>>> index 58c4f5e8a67e..5c72784a0b4d 100644 >>>>>>>> --- a/fs/overlayfs/export.c >>>>>>>> +++ b/fs/overlayfs/export.c >>>>>>>> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, >>>>>>>> return type; >>>>>>>> } >>>>>>>> >>>>>>>> +/* >>>>>>>> + * Find or instantiate an overlay dentry from real dentries. >>>>>>>> + */ >>>>>>>> +static struct dentry *ovl_obtain_alias(struct super_block *sb, >>>>>>>> + struct dentry *upper, >>>>>>>> + struct ovl_path *lowerpath) >>>>>>>> +{ >>>>>>>> + struct inode *inode; >>>>>>>> + struct dentry *dentry; >>>>>>>> + struct ovl_entry *oe; >>>>>>>> + >>>>>>>> + /* TODO: obtain non pure-upper */ >>>>>>>> + if (lowerpath) >>>>>>>> + return ERR_PTR(-EIO); >>>>>>>> + >>>>>>>> + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); >>>>>>>> + if (IS_ERR(inode)) { >>>>>>>> + dput(upper); >>>>>>>> + return ERR_CAST(inode); >>>>>>>> + } >>>>>>>> + >>>>>>>> + dentry = d_obtain_alias(inode); >>>>>>>> + if (IS_ERR(dentry) || dentry->d_fsdata) >>>>>>> >>>>>>> Racing two instances of this code, each thinking it got a new alias >>>>>>> and trying to fill it, results in a memory leak. >>>>>>> >>>>>>> Haven't checked in too much depth, but apparently other filesystems >>>>>>> are not affected, so we need something special here. >>>>>>> >>>>>>> One solution: split d_instantiate_anon(dentry, inode) out of >>>>>>> __d_obtain_alias() and supply that with the already initialized >>>>>>> dentry. >>>>>>> >>>>>> >>>>>> Can't we use &OVL_I(inode)->lock to avoid the race? >>>>> >>>>> We could. But then d_splice_alias() will find our half baked dentry >>>>> and return that from ovl_lookup(). >>>> >>>> No it won't, because we do not obtain dir dentries this way. >>>> We actually do in this patch [3/17], but since patch [4/17] we don't, >>>> so I only need to fix this patch not to obtain dir dentry and to >>>> protect concurrent decode of non-dir with &OVL_I(inode)->lock. >>>> >>>>> So we do need to have the dentry >>>>> fully initialized by the time it's added into the inode's alias list. >>>>> >>>> >>>> The only problems I see with adding a non-dir disconnected alias >>>> that is not fully baked are: >>>> 1. We can get it in ovl_encode_inode_fh() from d_find_any_alias() >>>> 2. nfsd can get it in exportfs_decode_fh() from find_acceptable_alias() >>>> in a weird hypothetical case where the fully baked dentry we just >>>> returned from ovl_obtain_alias() in NOT acceptable by nfsd but >>>> the half baked dentry IS acceptable >>>> 3. Another kernel user that uses d_find_any_alias() or one of the use >>>> case that only Al can think of... >>>> >>>> Cases 2 and 3, I don't know if they are for real. >>>> >>>> Case 1 is only a problem due to lack of export_operations method >>>> 'dentry_to_fh'. exportfs_encode_fh() has the right dentry, but it does >>>> not pass it to the filesystem for encoding, so I think it should be >>>> solved by adding this method. >>> >>> I agree with your analysis. >>> >>> However, I don't see what's wrong with adding fully baked dentries to >>> the inode. >> >> I agree that adding half baked dentries is not a good practice and >> we should avoid it. >> > > How is this for an option? > > =========================================== > > +/* > + * Find or instantiate an overlay dentry from real dentries. > + */ > +static struct dentry *ovl_obtain_alias(struct super_block *sb, > + struct dentry *upper, > + struct ovl_path *lowerpath) > +{ > + struct inode *inode; > + struct dentry *dentry; > + struct ovl_entry *oe; > + void *fsdata = &oe; > + > + /* TODO: obtain non pure-upper */ > + if (lowerpath) > + return ERR_PTR(-EIO); > + > + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); > + if (IS_ERR(inode)) { > + dput(upper); > + return ERR_CAST(inode); > + } > + With optimistic find: + /* First try our luck to find a cached dentry */ + dentry = d_find_any_alias(inode); + if (dentry) { + iput(inode); + return dentry; + } + + /* Then allocate ovl_entry, but free it if we do find a cached dentry */ > + oe = ovl_alloc_entry(0); > + if (!oe) { > + iput(inode); > + return ERR_PTR(-ENOMEM); > + } > + oe->has_upper = true; > + > + dentry = d_obtain_alias_fsdata(inode, fsdata); > + /* A new allocated dentry assigns *fsdata and sets it to NULL */ > + if (oe) > + kfree(oe); > + > + return dentry; > +} > + > > > ... > > > -static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) > +static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected, > + void **fsdata) > { > struct dentry *tmp; > struct dentry *res; > @@ -1962,6 +1963,12 @@ static struct dentry *__d_obtain_alias(struct > inode *inode, int disconnected) > if (disconnected) > add_flags |= DCACHE_DISCONNECTED; > > + /* Take ownership of pre-allocated fs-specific data */ > + if (fsdata) { > + tmp->d_fsdata = fsdata; And without the bug: + tmp->d_fsdata = *fsdata; > + *fsdata = NULL; > + } > + > spin_lock(&tmp->d_lock); > __d_set_inode_and_type(tmp, inode, add_flags); > hlist_add_head(&tmp->d_u.d_alias, &inode->i_dentry); > @@ -1998,7 +2005,13 @@ static struct dentry *__d_obtain_alias(struct > inode *inode, int disconnected) > */ > struct dentry *d_obtain_alias(struct inode *inode) > { > - return __d_obtain_alias(inode, 1); > + return __d_obtain_alias(inode, 1, NULL); > +} > +EXPORT_SYMBOL(d_obtain_alias); > + > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) > +{ > + return __d_obtain_alias(inode, 1, fsdata); > } > EXPORT_SYMBOL(d_obtain_alias); > > @@ -2019,7 +2032,7 @@ EXPORT_SYMBOL(d_obtain_alias); > */ > struct dentry *d_obtain_root(struct inode *inode) > { > - return __d_obtain_alias(inode, 0); > + return __d_obtain_alias(inode, 0, NULL); > } > EXPORT_SYMBOL(d_obtain_root);
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c index 58c4f5e8a67e..5c72784a0b4d 100644 --- a/fs/overlayfs/export.c +++ b/fs/overlayfs/export.c @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len, return type; } +/* + * Find or instantiate an overlay dentry from real dentries. + */ +static struct dentry *ovl_obtain_alias(struct super_block *sb, + struct dentry *upper, + struct ovl_path *lowerpath) +{ + struct inode *inode; + struct dentry *dentry; + struct ovl_entry *oe; + + /* TODO: obtain non pure-upper */ + if (lowerpath) + return ERR_PTR(-EIO); + + inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0); + if (IS_ERR(inode)) { + dput(upper); + return ERR_CAST(inode); + } + + dentry = d_obtain_alias(inode); + if (IS_ERR(dentry) || dentry->d_fsdata) + return dentry; + + oe = ovl_alloc_entry(0); + if (!oe) { + dput(dentry); + return ERR_PTR(-ENOMEM); + } + + dentry->d_fsdata = oe; + ovl_dentry_set_upper_alias(dentry); + + return dentry; +} + +static struct dentry *ovl_upper_fh_to_d(struct super_block *sb, + struct ovl_fh *fh) +{ + struct ovl_fs *ofs = sb->s_fs_info; + struct dentry *dentry; + struct dentry *upper; + + if (!ofs->upper_mnt) + return ERR_PTR(-EACCES); + + upper = ovl_decode_fh(fh, ofs->upper_mnt); + if (IS_ERR_OR_NULL(upper)) + return upper; + + dentry = ovl_obtain_alias(sb, upper, NULL); + dput(upper); + + return dentry; +} + +static struct dentry *ovl_fh_to_dentry(struct super_block *sb, struct fid *fid, + int fh_len, int fh_type) +{ + struct dentry *dentry = NULL; + struct ovl_fh *fh = (struct ovl_fh *) fid; + int len = fh_len << 2; + unsigned int flags = 0; + int err; + + err = -EINVAL; + if (fh_type != OVL_FILEID) + goto out_err; + + err = ovl_check_fh_len(fh, len); + if (err) + goto out_err; + + /* TODO: decode non-upper */ + flags = fh->flags; + if (flags & OVL_FH_FLAG_PATH_UPPER) + dentry = ovl_upper_fh_to_d(sb, fh); + err = PTR_ERR(dentry); + if (IS_ERR(dentry) && err != -ESTALE) + goto out_err; + + return dentry; + +out_err: + pr_warn_ratelimited("overlayfs: failed to decode file handle (len=%d, type=%d, flags=%x, err=%i)\n", + len, fh_type, flags, err); + return ERR_PTR(err); +} + const struct export_operations ovl_export_operations = { .encode_fh = ovl_encode_inode_fh, + .fh_to_dentry = ovl_fh_to_dentry, }; diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index a69cedf06000..87d39384dc55 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -107,7 +107,7 @@ static int ovl_acceptable(void *ctx, struct dentry *dentry) * Return -ENODATA for "origin unknown". * Return <0 for an invalid file handle. */ -static int ovl_check_fh_len(struct ovl_fh *fh, int fh_len) +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; @@ -171,7 +171,7 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry) goto out; } -static struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt) +struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt) { struct dentry *origin; int bytes; diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index f6fd999cb98e..c4f8e98e209e 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -258,6 +258,8 @@ static inline bool ovl_is_impuredir(struct dentry *dentry) /* namei.c */ +int ovl_check_fh_len(struct ovl_fh *fh, int fh_len); +struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt); int ovl_verify_origin(struct dentry *dentry, struct dentry *origin, bool is_upper, bool set); int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
Decoding an upper file handle is done by decoding the upper dentry from underlying upper fs, finding or allocating an overlay inode that is hashed by the real upper inode and instantiating an overlay dentry with that inode. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/export.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ fs/overlayfs/namei.c | 4 +-- fs/overlayfs/overlayfs.h | 2 ++ 3 files changed, 95 insertions(+), 2 deletions(-)