Bug 110491

Summary:	overlayfs: getcwd fails after trying to rmdir non-empty current working dir
Product:	File System	Reporter:	ncopa
Component:	Other	Assignee:	fs_other
Status:	NEW ---
Severity:	normal	CC:	admwiggin+kernelbugzilla, lars, rui.y.wang, szg00000
Priority:	P1
Hardware:	All
OS:	Linux
Kernel Version:	4.1.15	Subsystem:
Regression:	No	Bisected commit-id:
Attachments:	propsed patch

Description ncopa 2016-01-07 09:01:04 UTC

To reproduce:

1. mount overlay fs
2. create a non-empty directory
3. chdir to the non-empty directory
4. try rmdir the non-empty current working directory (should fail due to non-empty)
5. call getcwd. The call will fail with "No such file or directory", but it should not, becuase the directory was not successfully removed.

Example:

# mkdir lower upper work mnt
# mount -t overlay -o upperdir=upper,lowerdir=lower,workdir=work overlay mnt
# mkdir -p mnt/a/b
# cd mnt/a
# rmdir ../a
rmdir: failed to remove ‘../a’: Directory not empty
ash: getcwd: No such file or directory

Comment 1 ncopa 2016-01-07 09:02:29 UTC

More details: https://github.com/docker/docker/issues/19082

Comment 2 Rui Wang 2016-01-09 01:16:19 UTC

Created attachment 199071 [details]
propsed patch

Comment 3 Rui Wang 2016-01-09 01:17:24 UTC

A patch is proposed at: https://lkml.org/lkml/2016/1/8/482

I've also attached it here.

Comment 4 ncopa 2016-01-11 13:02:48 UTC

Did you test the patch? It does not seem to solve the issue for me.

I am not familiar with the linux filesystem code, but I would have expected it to also be dropped in the -ESTALE case?

Also, will this introduce a leak?

Comment 5 Rui Wang 2016-01-12 00:41:22 UTC

Yes I did test it. d_drop() isn't called in the -ESTALE case. It's if(!err), not if (err). d_drop() is called only if err is zero. What kind of leak do you think it may introduce?

Thanks

Comment 6 Rui Wang 2016-01-12 09:18:16 UTC

(In reply to ncopa from comment #4)
> Did you test the patch? It does not seem to solve the issue for me.
> 
> I am not familiar with the linux filesystem code, but I would have expected
> it to also be dropped in the -ESTALE case?
> 
> Also, will this introduce a leak?

I see. The original code do d_drop() in the -ESTALE case, but this should be unrelated. If this doesn't work then there may be another bug somewhere, e.g. your code path does not call ovl_remove_upper()? How did you reproduce the problem with this patch compiled? I tried it on overlayfs over xfs/ext4/ext3 by following your steps. All worked.

Comment 7 ncopa 2016-01-12 10:55:12 UTC

(In reply to Rui Wang from comment #6)
> (In reply to ncopa from comment #4)
> > Did you test the patch? It does not seem to solve the issue for me.
> > 
> > I am not familiar with the linux filesystem code, but I would have expected
> > it to also be dropped in the -ESTALE case?
> > 
> > Also, will this introduce a leak?
> 
> I see. The original code do d_drop() in the -ESTALE case, but this should be
> unrelated. If this doesn't work then there may be another bug somewhere,
> e.g. your code path does not call ovl_remove_upper()? How did you reproduce
> the problem with this patch compiled? I tried it on overlayfs over
> xfs/ext4/ext3 by following your steps. All worked.

I must have done something wrong during my test and tested wrong version.

It works. Tested on overlay over tmpfs.

> What kind of leak do you think it may introduce?

I don't know. I am not familiar with the filesystem code. It just looks that something that previously was dropped (possibly free'ed/released?) no longer is, so I just want be sure.

But if you know that no leak was introduced, then I'm all happy :)

I will push this for the Alpine Linux kernel.

Thanks!

Comment 8 Rui Wang 2016-01-13 10:15:11 UTC

(In reply to ncopa from comment #7)

> > What kind of leak do you think it may introduce?
> 
> I don't know. I am not familiar with the filesystem code. It just looks that
> something that previously was dropped (possibly free'ed/released?) no longer
> is, so I just want be sure.
> 
> But if you know that no leak was introduced, then I'm all happy :)

I think there should be no problem. -ESTALE means it's already deleted. Then it has been droped already and shouldn't really be still on the hash queue. 

> I will push this for the Alpine Linux kernel.

Cool. Thanks.