NACK: [SRU][UNSTABLE/FOCAL/BIONIC][PATCH v2] UBUNTU: SAUCE: shiftfs: fix dentry revalidation

[Stefan Bader]

On 18.05.20 20:18, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1879196
> 
> While fixing [1] we introduced a regression and triggered [2] whereby
> the underlay and overlay go out of sync because they disagree about the
> dcache. Such situation easily arise when the underlay is modified
> directly. This means the overlay needs to revalidate the dentry in the
> dcache to catchup with the underlay.
> 
> Note, in general it's not advisable to directly modify the underlay
> while a shiftfs mount is on top. In some way this means we need to keep
> two caches in sync and it's hard enough to keep a single cache happy.
> But shiftfs' use-case is inherently prone to be used for exactly that.
> So this is something we have to navigate carefully and honestly we have
> no full model upstream that does the same. Overlayfs has the copy-up
> behavior which let's it get around most of the issues but we don't have
> it and ecryptfs is broken in such scenarios which we verified quite a
> while back.
> In any case, I built a kernel with this patch and re-ran all regressions
> that are related to this that we have so far (cf.  [1], [2], and [3]).
> None of them were reproducible with this patch here. So we still fix the
> ESTALE issue but also keep underlay and overlay in sync.

This test kernel should be provided to the bug reporter, so it can be confirmed
to fix the issue there as well.
But more importantly, you submitted this for Bionic and there is no shiftfs in
that release. Only Eoan. I am not convinced you tried at least to apply the
patch to all the releases you submit for. There is always chances that code had
to be adjusted to be backported. Or that it applies but no longer compiles
because added function calls do not exist there.

-Stefan
> 
> [1]: https://bugs.launchpad.net/bugs/1872757
> [2]: https://bugs.launchpad.net/bugs/1879196
> [3]: https://bugs.launchpad.net/bugs/1860041
> Cc: Seth Forshee <seth.forshee at canonical.com>
> Cc: James Troup <james.troup at canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> ---
>  fs/shiftfs.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 39b878a6f820..652fb67b0e0b 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -240,6 +240,9 @@ static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
>  	int err = 1;
>  	struct dentry *lowerd = dentry->d_fsdata;
>  
> +	if (d_is_negative(lowerd) != d_is_negative(dentry))
> +		return 0;
> +
>  	if (lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
>  		err = lowerd->d_op->d_weak_revalidate(lowerd, flags);
>  		if (err < 0)
> @@ -264,13 +267,16 @@ static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
>  	if (flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> +	if (d_unhashed(lowerd) ||
> +	    d_is_negative(lowerd) != d_is_negative(dentry))
> +		return 0;
> +
>  	if (lowerd->d_flags & DCACHE_OP_REVALIDATE) {
>  		err = lowerd->d_op->d_revalidate(lowerd, flags);
>  		if (err < 0)
>  			return err;
>  		if (!err) {
> -			if (!(flags & LOOKUP_RCU))
> -				d_invalidate(lowerd);
> +			d_invalidate(lowerd);
>  			return -ESTALE;
>  		}
>  	}
> 
> base-commit: 9dd531622eff2baf657b906293745ca6dab9bb91
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200519/76532386/attachment.sig>