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. 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) 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
