ACK[U/F/E]/NAK[B/X]/cmnt: [SRU][UNSTABLE/BIONIC/XENIAL][[PATCH v2] UBUNTU: SAUCE: shiftfs: fix dentry revalidation

[Kleber Souza]

On 23.04.20 15:15, Kleber Souza wrote:
> On 14.04.20 22:26, Christian Brauner wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1872757
>>
>> Our revalidate methods were very opinionated about whether or not a
>> dentry was valid when we really should've just let the underlay tell us
>> what's what. This has led to bugs where a ESTALE was returned for e.g.
>> temporary files that were created and directly re-opened afterwards
>> through /proc/<pid>/fd/<nr-of-deleted-file>. When a file is re-opened
>> through /proc/<pid>/fd/<nr> LOOKUP_JUMP is set and the vfs will
>> revalidate via d_weak_revalidate(). Since the file has been unhashed or
>> even already gone negative we'd fail the open when we should've
>> succeeded.
>>
>> I had also foolishly provided a .tmpfile method which so far only has
>> caused us trouble. If we really need this then we can reimplement it
>> properly but I doubt it. Remove it for now.
>>
>> Reported-by: Christian Kellner <ckellner at redhat.com>
>> Reported-by: Evgeny Vereshchagin <evvers at ya.ru>
>> Cc: Seth Forshee <seth.forshee at canonical.com>
>> Link: https://github.com/systemd/systemd/issues/14861
>> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> 
> For Unstable:
> 
> Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> 
> 
> Bionic and Xenial don't have shiftfs, should this go to Focal and Eoan
> instead?

Please apply this to Focal and Eoan as well as pointed out by Christian
on the other thread.


Thanks,
Kleber


> 
> 
> Thanks,
> Kleber
> 
>> ---
>>  fs/shiftfs.c | 45 +++++++++++++--------------------------------
>>  1 file changed, 13 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
>> index 5c39529d0a17..39b878a6f820 100644
>> --- a/fs/shiftfs.c
>> +++ b/fs/shiftfs.c
>> @@ -240,19 +240,17 @@ 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))
>> +	if (lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
>>  		err = lowerd->d_op->d_weak_revalidate(lowerd, flags);
>> +		if (err < 0)
>> +			return err;
>> +	}
>>  
>>  	if (d_really_is_positive(dentry)) {
>>  		struct inode *inode = d_inode(dentry);
>>  		struct inode *loweri = d_inode(lowerd);
>>  
>>  		shiftfs_copyattr(loweri, inode);
>> -		if (!inode->i_nlink)
>> -			err = 0;
>>  	}
>>  
>>  	return err;
>> @@ -263,23 +261,25 @@ static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
>>  	int err = 1;
>>  	struct dentry *lowerd = dentry->d_fsdata;
>>  
>> -	if (d_unhashed(lowerd) ||
>> -	    ((d_is_negative(lowerd) != d_is_negative(dentry))))
>> -		return 0;
>> -
>>  	if (flags & LOOKUP_RCU)
>>  		return -ECHILD;
>>  
>> -	if ((lowerd->d_flags & DCACHE_OP_REVALIDATE))
>> +	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);
>> +			return -ESTALE;
>> +		}
>> +	}
>>  
>>  	if (d_really_is_positive(dentry)) {
>>  		struct inode *inode = d_inode(dentry);
>>  		struct inode *loweri = d_inode(lowerd);
>>  
>>  		shiftfs_copyattr(loweri, inode);
>> -		if (!inode->i_nlink)
>> -			err = 0;
>>  	}
>>  
>>  	return err;
>> @@ -736,24 +736,6 @@ static int shiftfs_fiemap(struct inode *inode,
>>  	return err;
>>  }
>>  
>> -static int shiftfs_tmpfile(struct inode *dir, struct dentry *dentry,
>> -			   umode_t mode)
>> -{
>> -	int err;
>> -	const struct cred *oldcred;
>> -	struct dentry *lowerd = dentry->d_fsdata;
>> -	struct inode *loweri = dir->i_private;
>> -
>> -	if (!loweri->i_op->tmpfile)
>> -		return -EOPNOTSUPP;
>> -
>> -	oldcred = shiftfs_override_creds(dir->i_sb);
>> -	err = loweri->i_op->tmpfile(loweri, lowerd, mode);
>> -	revert_creds(oldcred);
>> -
>> -	return err;
>> -}
>> -
>>  static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
>>  {
>>  	struct dentry *lowerd = dentry->d_fsdata;
>> @@ -1020,7 +1002,6 @@ static const struct inode_operations shiftfs_file_inode_operations = {
>>  	.listxattr	= shiftfs_listxattr,
>>  	.permission	= shiftfs_permission,
>>  	.setattr	= shiftfs_setattr,
>> -	.tmpfile	= shiftfs_tmpfile,
>>  };
>>  
>>  static const struct inode_operations shiftfs_special_inode_operations = {
>>
>> base-commit: 1ff0ece4a5b555e1a72ec8d030b9410761174598
>>
>