On Wed, Nov 09, 2022 at 11:16:23AM +0200, Amir Goldstein wrote: > On Wed, Nov 9, 2022 at 6:16 AM YoungJun.Park <her0gyugyu@xxxxxxxxx> wrote: > > > > On Tue, Nov 08, 2022 at 09:22:38PM +0200, Amir Goldstein wrote: > > > On Tue, Nov 8, 2022 at 2:37 PM YoungJun.Park <her0gyugyu@xxxxxxxxx> wrote: > > > > > > > > On Tue, Nov 08, 2022 at 11:22:14AM +0200, Amir Goldstein wrote: > > > > > On Tue, Nov 8, 2022, 10:14 AM YoungJun.Park <her0gyugyu@xxxxxxxxx> wrote: > > > > > > > > > > > On Mon, Nov 07, 2022 at 10:49:57AM +0200, Amir Goldstein wrote: > > > > > > > On Mon, Nov 7, 2022 at 9:06 AM YoungJun.Park <her0gyugyu@xxxxxxxxx> > > > > > > wrote: > > > > > > > > > > > > > > > > On Mon, Nov 07, 2022 at 08:40:02AM +0200, Amir Goldstein wrote: > > > > > > > > > On Mon, Nov 7, 2022 at 6:38 AM YoungJun.Park <her0gyugyu@xxxxxxxxx> > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Here is my curious scenario. > > > > > > > > > > > > > > > > > > > > 1. create a file on overlayfs. > > > > > > > > > > 2. delete a file on upper directory. > > > > > > > > > > 3. can see file contents using read sys call. (may file operations > > > > > > all success) > > > > > > > > > > 4. cannot remove, rename. it return -ESTALE error (may inode > > > > > > operations fail) > > > > > > > > > > > > > > > > > > > > I understand this scenario onto the code level. > > > > > > > > > > But I don't understand this situation itself. > > > > > > > > > > > > > > > > > > > > I found a overlay kernel docs and it comments > > > > > > > > > > Changes to underlying filesystems section > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > Changes to the underlying filesystems while part of a mounted > > > > > > overlay filesystem are not allowed. > > > > > > > > > > If the underlying filesystem is changed, the behavior of the > > > > > > overlay is undefined, > > > > > > > > > > though it will not result in a crash or deadlock. > > > > > > > > > > .... > > > > > > > > > > > > > > > > > > > > So here is my question (may it is suggestion) > > > > > > > > > > > > > > > > > > > > 1. underlying file system change is not allowed, then how about > > > > > > implementing shadow upper directory from user? > > > > > > > > > > 2. if read, write system call is allowed, how about changing > > > > > > remove, rename(and more I does not percept) operation success? > > > > > > > > > > > > > > > > > > > > > > > > > > > > What is your use case? > > > > > > > > > Why do you think this is worth spending time on? > > > > > > > > > If anything, we could implement revalidate to return ESTALE also > > > > > > from open > > > > > > > > > in such a case. > > > > > > > > > But again, why do you think that would matter? > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Amir. > > > > > > > > > > > > > > > > Thank you for replying. > > > > > > > > I develop antivirus scanner. > > > > > > > > When developing, I am confronted the situaion below. > > > > > > > > > > > > > > > > 1. make a docker container using overlayfs > > > > > > > > 2. our antivirus scanner detect on upperdir and remove it. > > > > > > > > 3. When I check container, the file contents can be read, buf file > > > > > > cannot be removed.(-ESTALE error) > > > > > > > > > > > > > > > > And as I think, the reason is upperdir is touchable. So it is better > > > > > > to hide upperdir. > > > > > > > > If it is hard to implement(or maybe there is a other reson that I don' > > > > > > know) > > > > > > > > it is better to make the situation is clear > > > > > > > > (file operation error, inode operations error or file operation > > > > > > success , inode operation success) > > > > > > > > > > > > > > > > > > > > > > Error on read is not an option because reading from an open and deleted > > > > > > > file is perfectly valid even without overlayfs. > > > > > > > > > > > > > > ESTALE error on open is doable and makes sense and I believe it may > > > > > > > be sufficient for your use case. > > > > > > > > > > > > > > I have an old branch that implements that behavior: > > > > > > > https://github.com/amir73il/linux/commits/ovl-revalidate > > > > > > > > > > > > > > You can try it out and see if that works for you. > > > > > > > If it does, I can post the patches. > > > > > > > > > > > > > > Note that the use case that you described does not need the last patch, > > > > > > > but if the anti-virus would have moved a lower file to quarantine > > > > > > > instead of deleting it, the last patch would also be useful for you. > > > > > > > > > > > > > > Thanks, > > > > > > > Amir. > > > > > > > > > > > > After applying the branch, I tested the scenario. > > > > > > But it does not work. file open is success on overlayfs filesystem. > > > > > > > > > > > > In my scnario, the dentry is not negative and just unhashed on upper. > > > > > > > > > > > > > > > > Yeh my bad. > > > > > I also noticed that after I sent you the link. > > > > > I think my patch also has a memleak somewhere I seen kmemleak reports > > > > > during testing. > > > > > > > > > > If we check dentry is unhashed we properly block open on my scenario. > > > > > > I write the patch and tested it working. > > > > > > (Maybe I does not catch your point, if you give a guide then I follow it) > > > > > > > > > > > > > > > > Please place the unhashed check inside ovl_revalidate_real() same as my > > > > > checks for negative upper and renamed lower dentry. > > > > > > > > > > The dentry should only be considered stale if the real dentry is unhashed > > > > > but ovl entry is hashed. > > > > > > > > > > The state of both ovl dentry and real dentry unhashed is possible and valid > > > > > I think, but it should not interfere with your use case where ovl dentry is > > > > > hashed and real upper is unhashed. > > > > > > > > > > I may be missing something so better if Miklos also takes a look at the > > > > > patch. > > > > > > > > > > Thanks, > > > > > Amir. > > > > > > > > > > > > > > > > Signed-off-by: YoungJun.park <her0gyugyu@xxxxxxxxx> > > > > > > --- > > > > > > fs/overlayfs/file.c | 4 ++++ > > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c > > > > > > index 6512d147c223..629dbcc49070 100644 > > > > > > --- a/fs/overlayfs/file.c > > > > > > +++ b/fs/overlayfs/file.c > > > > > > @@ -157,6 +157,10 @@ static int ovl_open(struct inode *inode, struct file > > > > > > *file) > > > > > > file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC); > > > > > > > > > > > > ovl_path_realdata(dentry, &realpath); > > > > > > + > > > > > > + if (d_unhashed(realpath.dentry)) > > > > > > + return -ESTALE; > > > > > > + > > > > > > realfile = ovl_open_realfile(file, &realpath); > > > > > > if (IS_ERR(realfile)) > > > > > > return PTR_ERR(realfile); > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > And I have one more question. > > > > > > Why upper dir must be visible..? > > > > > > The reson I think making upper dir unvisible is like the below. > > > > > > 1. If making a upperdir is unvisible, then these kind of problem disappear. > > > > > > 2. upperdir visibility makes a passage to convey container's file to > > > > > > hostland. > > > > > > (in view of container using overlayfs) > > > > > > making unvisible remove this kind of problem. > > > > > > 3. Changing upper dir scenario makes undefined behavior. So, if removing > > > > > > the interface > > > > > > user can access, then we can make the undefined scenario itself. > > > > > > > > > > > > Thanks Amir. > > > > > > Best regards > > > > > > > > > > > > > > Thank you for kind guidance Amir. > > > > Before I will do next step, I want to recheck your point Amir :) > > > > > > > > 1. > > > > > Please place the unhashed check inside ovl_revalidate_real() same as my > > > > > checks for negative upper and renamed lower dentry. > > > > > > > > You mean I modify your commit? It is from your branch then > > > > How I fix it...? (Am I misunderstood somthing?) > > > > here is pseudo patch. > > > > > > My branch is just a POC. > > > If you test it and say that it is useful for you I can post it > > > and add your use case as the motivation. > > > > > > > > > > > invalidate dentry if dentry is unhashed > > > > > > > > Signed-off-by: YoungJun.park <her0gyugyu@xxxxxxxxx> > > > > --- > > > > fs/overlayfs/super.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > > > index 6a4f2b87f1a3..411d3ed8aec1 100644 > > > > --- a/fs/overlayfs/super.c > > > > +++ b/fs/overlayfs/super.c > > > > @@ -129,8 +129,8 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak, > > > > { > > > > int ret = 1; > > > > > > > > - /* Invalidate dentry if real was deleted/renamed since we found it */ > > > > - if (ovl_dentry_is_dead(d) || (hash && hash != d->d_name.hash_len)) { > > > > + /* Invalidate dentry if real was deleted/renamed/unhashed since we found it */ > > > > + if (ovl_dentry_is_dead(d) || (hash && hash != d->d_name.hash_len) || d_unhashed(d)) { > > > > ret = 0; > > > > } else if (weak) { > > > > if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE) > > > > -- > > > > 2.25.1 > > > > > > > > > > That won't be enough and is also not accurate. > > > I pushed another patch to branch ovl-revalidate and also > > > tested that your use case works as expected when > > > deleting either upper or lower non-dir files. > > > > > > The patch adds this functionality unconditionally for non-dir, > > > but it may be required to use some mount option to enable it. > > > It is up to Miklos to decide. > > FYI, I changed the patch to enable this functionality based > on new features. > > So if you pull the POC branch, to enable the strict checks you > need to enable either redirect_dir or metacopy, i.e.: > > echo Y > /sys/module/overlay/parameters/redirect_dir > echo Y > /sys/module/overlay/parameters/metacopy > > at runtime or > > CONFIG_OVERLAY_FS_REDIRECT_DIR=Y > CONFIG_OVERLAY_FS_METACOPY=Y > > at build time. > > > > > > > > 2. > > > > > > > > > The dentry should only be considered stale if the real dentry is unhashed > > > > > but ovl entry is hashed. > > > > > > > > > > The state of both ovl dentry and real dentry unhashed is possible and valid > > > > > I think, but it should not interfere with your use case where ovl dentry is > > > > > hashed and real upper is unhashed. > > > > > > > > > > I may be missing something so better if Miklos also takes a look at the > > > > > patch. > > > > > > > > > > > > > You mean, if I modify the code you said, then the patch I sent works properly? (file open fail) > > > > If you mean it, then I post my patch :) > > > > > > > > > > Please test the patch that I pushed to branch ovl-revalidate. > > > > > > > And the last question, I really curious "hide upperdir from user" idea. If it is meaningful > > > > I want to try to implement it, If it isn't then could you explain why this idea is not meaningful..? > > > > > > > > > > It is not meaningful, it is not relevant. > > > There is no way to hide a directory. > > > You can bind mount an empty dir over it in one mount namespace > > > but in other mount namespaces or other bind mounts that dir will be visible. > > > > > > Thanks, > > > Amir. > > > > Thank you Amir > > I tested the branch and check the -ENOENT is returned on open time in my use case. > > And I have one more suggestion, it is would be better on upper file rename case to be covered as well. > > The idea is pre query dentry hash on ovl_inode initialization time. > > If it is acceptable, then consider to apply this idea. > > here is my patch. > > This patch is wrong. > Renaming a file from overlay (not directly from upper dir) will also result in > error trying to access the renamed file. > > See my commit message on lower renames: > > "We do not provide this protection for upper dentries, because that would > require updating the hash on overlay initiated renames and that is harder > to implement with lockless lookup." > > Detecting renames in upper dir is doable, but it is more complicated. > > > > > Signed-off-by: YoungJun.park <her0gyugyu@xxxxxxxxx> > > --- > > fs/overlayfs/inode.c | 4 +++- > > fs/overlayfs/ovl_entry.h | 1 + > > fs/overlayfs/super.c | 2 +- > > 3 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > > index 9e61511de7a7..efed51608033 100644 > > --- a/fs/overlayfs/inode.c > > +++ b/fs/overlayfs/inode.c > > @@ -862,8 +862,10 @@ void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip, > > struct inode *realinode; > > struct ovl_inode *oi = OVL_I(inode); > > > > - if (oip->upperdentry) > > + if (oip->upperdentry) { > > oi->__upperdentry = oip->upperdentry; > > + oi->upper_hash = oip->upperdentry->d_name.hash_len; > > + } > > if (oip->lowerpath && oip->lowerpath->dentry) { > > oi->lowerpath.dentry = dget(oip->lowerpath->dentry); > > oi->lowerpath.layer = oip->lowerpath->layer; > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h > > index b83db8b6a31c..5a11f0a83436 100644 > > --- a/fs/overlayfs/ovl_entry.h > > +++ b/fs/overlayfs/ovl_entry.h > > @@ -135,6 +135,7 @@ struct ovl_inode { > > u64 version; > > unsigned long flags; > > struct inode vfs_inode; > > + u64 upper_hash; > > struct dentry *__upperdentry; > > struct ovl_path lowerpath; > > > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > index 59d5e3147a50..d84e34515d7c 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -172,7 +172,7 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry, > > > > upper = ovl_dentry_upper(dentry); > > if (upper) > > - ret = ovl_revalidate_real(upper, flags, weak, 0); > > + ret = ovl_revalidate_real(upper, flags, weak, OVL_I(d_inode(upper))->upper_hash); > > > > for (i = 0; ret > 0 && i < oe->numlower; i++, lower++) { > > ret = ovl_revalidate_real(lower->dentry, flags, weak, > > -- > > 2.25.1 > > > > > It is not meaningful, it is not relevant. > > > There is no way to hide a directory. > > > You can bind mount an empty dir over it in one mount namespace > > > but in other mount namespaces or other bind mounts that dir will be visible. > > > > And this comment > > Um, As I think, I don't know advantage of upperdir reachable (or writable) > > So, I think hide of upperdir from user can make this situation does not happen. > > And there is other way to make upperdir readable or semi hide state from user(as you say) > > but it is not forcefully applied by overlayfs and user has a responsibility of doing that. > > (many user does not care and know well this features) > > I think hiding upperdir, making upperdir readable forcefully and any idea of blocking touchable > > upperdir can make overlayfs happy. > > > > I don't know how to explain why this is not doable. > > I have another POC > https://github.com/amir73il/linux/commits/ovl-watch > > Where overlayfs uses fsnotify to monitor changes to upper/lower dirs. > This could be used to deny direct changes to upper/lower dirs, > but there are many subtle details and I am not pursuing this POC > at the moment. > > Thanks, > Amir. I clearly understand the things you explain :) Thanks. YoungJun.
