[PATCH v3 0/4] Overlayfs mkdir related fixes

[PATCH v3 0/4] Overlayfs mkdir related fixes

[Amir Goldstein]

Miklos,

This is v3 of mkdir fixes, which addresses comments from Vivek
and yourself as well as fix some issues found in tests of v2.
The series is based and tested on top of a merge of overlayfs-rorw
and Al's fixes branch.

Thanks,
Amir.

Changes since v2:
- Don't use d_instantiate_new() helper
- Explain inode hash race (Vivek)
- Relax WARN_ON() to pass overlay/019 test
- Create hepler ovl_create_real_dir()
- pr_warn() when real mkdir succeeds and lookup fails

Changes since v1:
- Rebase on top of Al's fixes branch
- Use d_instantiate_new() helper
- Add re-factoring + Al's vfs_mkdir() safely patch

Al Viro (1):
  ovl: make ovl_create_real() cope with vfs_mkdir() safely

Amir Goldstein (3):
  ovl: use insert_inode_locked4() to hash a newly created inode
  ovl: relax WARN_ON() real inode attributes mismatch
  ovl: create helper ovl_create_temp()

 fs/overlayfs/copy_up.c   |  27 +++-------
 fs/overlayfs/dir.c       | 125 +++++++++++++++++++++++++++++++++++------------
 fs/overlayfs/inode.c     |  18 +++++++
 fs/overlayfs/overlayfs.h |  12 ++++-
 fs/overlayfs/super.c     |   2 +-
 5 files changed, 130 insertions(+), 54 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

Currently, there is a small window where ovl_obtain_alias() can
race with ovl_instantiate() and create two different overlay inodes
with the same underlying real non-dir non-hardlink inode.

The race requires an adversary to guess the file handle of the
yet to be created upper inode and decode the guessed file handle
after ovl_creat_real(), but before ovl_instantiate().

This patch fixes the race, by using insert_inode_locked4() to add
a newly created inode to icache.

If the newly created inode apears to already exist in icache (hashed
by the same real upper inode), we export this error to user instead
of silently not hashing the new inode.

This race does not affect overlay directory inodes, because those
are decoded via ovl_lookup_real() and not with ovl_obtain_alias(),
so avoid using the new helper d_instantiate_new() to reduce backport
dependencies.

Backporting only makes sense for v4.16 where NFS export was introduced.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: <stable@vger.kernel.org> #v4.16
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 24 ++++++++++++++++++------
 fs/overlayfs/inode.c     | 18 ++++++++++++++++++
 fs/overlayfs/overlayfs.h |  1 +
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 47dc980e8b33..62e6733b755c 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -183,14 +183,24 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
 }
 
 /* Common operations required to be done after creation of file on upper */
-static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
-			    struct dentry *newdentry, bool hardlink)
+static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
+			   struct dentry *newdentry, bool hardlink)
 {
 	ovl_dir_modified(dentry->d_parent, false);
-	ovl_copyattr(d_inode(newdentry), inode);
 	ovl_dentry_set_upper_alias(dentry);
 	if (!hardlink) {
-		ovl_inode_update(inode, newdentry);
+		int err;
+
+		ovl_inode_init(inode, newdentry, NULL);
+		/*
+		 * XXX: if we ever use ovl_obtain_alias() to decode directory
+		 * file handles, need to use ovl_insert_inode_locked() and
+		 * d_instantiate_new() here to prevent ovl_obtain_alias()
+		 * from sneaking in before d_instantiate().
+		 */
+		err = ovl_insert_inode(inode, d_inode(newdentry));
+		if (err)
+			return err;
 	} else {
 		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
 		dput(newdentry);
@@ -200,6 +210,8 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
 	/* Force lookup of new upper hardlink to find its lower */
 	if (hardlink)
 		d_drop(dentry);
+
+	return 0;
 }
 
 static bool ovl_type_merge(struct dentry *dentry)
@@ -238,7 +250,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 		ovl_set_opaque(dentry, newdentry);
 	}
 
-	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
+	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
 	newdentry = NULL;
 out_dput:
 	dput(newdentry);
@@ -439,7 +451,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		if (err)
 			goto out_cleanup;
 	}
-	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
+	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
 	newdentry = NULL;
 out_dput2:
 	dput(upper);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 7abcf96e94fc..060c534998d1 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -741,6 +741,24 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
 	return true;
 }
 
+static int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode)
+{
+	return insert_inode_locked4(inode, (unsigned long) realinode,
+				    ovl_inode_test, realinode);
+}
+
+int ovl_insert_inode(struct inode *inode, struct inode *realinode)
+{
+	int err;
+
+	err = ovl_insert_inode_locked(inode, realinode);
+	if (err)
+		return err;
+
+	unlock_new_inode(inode);
+	return 0;
+}
+
 struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
 			       bool is_upper)
 {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index caaa47cea2aa..642b25702092 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
 bool ovl_is_private_xattr(const char *name);
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
+int ovl_insert_inode(struct inode *inode, struct inode *realinode);
 struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
 			       bool is_upper);
 struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
-- 
2.7.4

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Vivek Goyal]

On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
> Currently, there is a small window where ovl_obtain_alias() can
> race with ovl_instantiate() and create two different overlay inodes
> with the same underlying real non-dir non-hardlink inode.
> 
> The race requires an adversary to guess the file handle of the
> yet to be created upper inode and decode the guessed file handle
> after ovl_creat_real(), but before ovl_instantiate().
> 
> This patch fixes the race, by using insert_inode_locked4() to add
> a newly created inode to icache.
> 
> If the newly created inode apears to already exist in icache (hashed
> by the same real upper inode), we export this error to user instead
> of silently not hashing the new inode.

So we might return an error to user saying operation failed, but still
create file on upper. Does that sound little odd?

I am wondering why can't we call ovl_get_inode() in object creation
path. That should take care of race between creation path and file
handle decode and only one of the paths will get to instantiate and
initialize ovl_inode and other path will wait.

Thanks
Vivek


> 
> This race does not affect overlay directory inodes, because those
> are decoded via ovl_lookup_real() and not with ovl_obtain_alias(),
> so avoid using the new helper d_instantiate_new() to reduce backport
> dependencies.
> 
> Backporting only makes sense for v4.16 where NFS export was introduced.
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: <stable@vger.kernel.org> #v4.16
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/dir.c       | 24 ++++++++++++++++++------
>  fs/overlayfs/inode.c     | 18 ++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  1 +
>  3 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 47dc980e8b33..62e6733b755c 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -183,14 +183,24 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
>  }
>  
>  /* Common operations required to be done after creation of file on upper */
> -static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
> -			    struct dentry *newdentry, bool hardlink)
> +static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
> +			   struct dentry *newdentry, bool hardlink)
>  {
>  	ovl_dir_modified(dentry->d_parent, false);
> -	ovl_copyattr(d_inode(newdentry), inode);
>  	ovl_dentry_set_upper_alias(dentry);
>  	if (!hardlink) {
> -		ovl_inode_update(inode, newdentry);
> +		int err;
> +
> +		ovl_inode_init(inode, newdentry, NULL);
> +		/*
> +		 * XXX: if we ever use ovl_obtain_alias() to decode directory
> +		 * file handles, need to use ovl_insert_inode_locked() and
> +		 * d_instantiate_new() here to prevent ovl_obtain_alias()
> +		 * from sneaking in before d_instantiate().
> +		 */
> +		err = ovl_insert_inode(inode, d_inode(newdentry));
> +		if (err)
> +			return err;
>  	} else {
>  		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
>  		dput(newdentry);
> @@ -200,6 +210,8 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
>  	/* Force lookup of new upper hardlink to find its lower */
>  	if (hardlink)
>  		d_drop(dentry);
> +
> +	return 0;
>  }
>  
>  static bool ovl_type_merge(struct dentry *dentry)
> @@ -238,7 +250,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>  		ovl_set_opaque(dentry, newdentry);
>  	}
>  
> -	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
> +	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
>  	newdentry = NULL;
>  out_dput:
>  	dput(newdentry);
> @@ -439,7 +451,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  		if (err)
>  			goto out_cleanup;
>  	}
> -	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
> +	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
>  	newdentry = NULL;
>  out_dput2:
>  	dput(upper);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 7abcf96e94fc..060c534998d1 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -741,6 +741,24 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
>  	return true;
>  }
>  
> +static int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode)
> +{
> +	return insert_inode_locked4(inode, (unsigned long) realinode,
> +				    ovl_inode_test, realinode);
> +}
> +
> +int ovl_insert_inode(struct inode *inode, struct inode *realinode)
> +{
> +	int err;
> +
> +	err = ovl_insert_inode_locked(inode, realinode);
> +	if (err)
> +		return err;
> +
> +	unlock_new_inode(inode);
> +	return 0;
> +}
> +
>  struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>  			       bool is_upper)
>  {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index caaa47cea2aa..642b25702092 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
>  bool ovl_is_private_xattr(const char *name);
>  
>  struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
> +int ovl_insert_inode(struct inode *inode, struct inode *realinode);
>  struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>  			       bool is_upper);
>  struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
> -- 
> 2.7.4
> 

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>> Currently, there is a small window where ovl_obtain_alias() can
>> race with ovl_instantiate() and create two different overlay inodes
>> with the same underlying real non-dir non-hardlink inode.
>>
>> The race requires an adversary to guess the file handle of the
>> yet to be created upper inode and decode the guessed file handle
>> after ovl_creat_real(), but before ovl_instantiate().
>>
>> This patch fixes the race, by using insert_inode_locked4() to add
>> a newly created inode to icache.
>>
>> If the newly created inode apears to already exist in icache (hashed
>> by the same real upper inode), we export this error to user instead
>> of silently not hashing the new inode.
>
> So we might return an error to user saying operation failed, but still
> create file on upper. Does that sound little odd?
>

Yes, but I don't see a better solution.

> I am wondering why can't we call ovl_get_inode() in object creation
> path. That should take care of race between creation path and file
> handle decode and only one of the paths will get to instantiate and
> initialize ovl_inode and other path will wait.
>

I don't even want to think if what you wrote makes sense.
Remember that the use case we are talking about is quite imaginary.
Ensuring internal structures consistency in our code and returning
error to user is the right thing to do for imaginary use cases IMO.

Thanks,
Amir.

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Miklos Szeredi]

On Tue, May 15, 2018 at 3:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>> Currently, there is a small window where ovl_obtain_alias() can
>>> race with ovl_instantiate() and create two different overlay inodes
>>> with the same underlying real non-dir non-hardlink inode.
>>>
>>> The race requires an adversary to guess the file handle of the
>>> yet to be created upper inode and decode the guessed file handle
>>> after ovl_creat_real(), but before ovl_instantiate().
>>>
>>> This patch fixes the race, by using insert_inode_locked4() to add
>>> a newly created inode to icache.
>>>
>>> If the newly created inode apears to already exist in icache (hashed
>>> by the same real upper inode), we export this error to user instead
>>> of silently not hashing the new inode.
>>
>> So we might return an error to user saying operation failed, but still
>> create file on upper. Does that sound little odd?
>>
>
> Yes, but I don't see a better solution.

Might be better to kick the other, offending inode out, instead of
returning an error.  It would also simplify the error handling.

We can do that by creating an ovl_inode_test_kick() variant that
unhashes the inode on match.  Also needs insert_inode_locked4() to use
hlist_for_each_entry_safe() instead of hlist_for_each_entry().

Thanks,
Miklos

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

On Wed, May 16, 2018 at 11:34 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, May 15, 2018 at 3:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>>> Currently, there is a small window where ovl_obtain_alias() can
>>>> race with ovl_instantiate() and create two different overlay inodes
>>>> with the same underlying real non-dir non-hardlink inode.
>>>>
>>>> The race requires an adversary to guess the file handle of the
>>>> yet to be created upper inode and decode the guessed file handle
>>>> after ovl_creat_real(), but before ovl_instantiate().
>>>>
>>>> This patch fixes the race, by using insert_inode_locked4() to add
>>>> a newly created inode to icache.
>>>>
>>>> If the newly created inode apears to already exist in icache (hashed
>>>> by the same real upper inode), we export this error to user instead
>>>> of silently not hashing the new inode.
>>>
>>> So we might return an error to user saying operation failed, but still
>>> create file on upper. Does that sound little odd?
>>>
>>
>> Yes, but I don't see a better solution.
>
> Might be better to kick the other, offending inode out, instead of
> returning an error.  It would also simplify the error handling.
>
> We can do that by creating an ovl_inode_test_kick() variant that
> unhashes the inode on match.  Also needs insert_inode_locked4() to use
> hlist_for_each_entry_safe() instead of hlist_for_each_entry().
>

Do you really think that this corner use case calls for such actions,
as creating flavors of inode cache helpers?
Remember that the so called "offending" inode, is not offending in
a way that is wrong or incomplete in any way.

So worst worst worst case this is a very temporal error.

Thanks,
Amir.

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Miklos Szeredi]

On Wed, May 16, 2018 at 11:51 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, May 16, 2018 at 11:34 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, May 15, 2018 at 3:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>>>> Currently, there is a small window where ovl_obtain_alias() can
>>>>> race with ovl_instantiate() and create two different overlay inodes
>>>>> with the same underlying real non-dir non-hardlink inode.
>>>>>
>>>>> The race requires an adversary to guess the file handle of the
>>>>> yet to be created upper inode and decode the guessed file handle
>>>>> after ovl_creat_real(), but before ovl_instantiate().
>>>>>
>>>>> This patch fixes the race, by using insert_inode_locked4() to add
>>>>> a newly created inode to icache.
>>>>>
>>>>> If the newly created inode apears to already exist in icache (hashed
>>>>> by the same real upper inode), we export this error to user instead
>>>>> of silently not hashing the new inode.
>>>>
>>>> So we might return an error to user saying operation failed, but still
>>>> create file on upper. Does that sound little odd?
>>>>
>>>
>>> Yes, but I don't see a better solution.
>>
>> Might be better to kick the other, offending inode out, instead of
>> returning an error.  It would also simplify the error handling.
>>
>> We can do that by creating an ovl_inode_test_kick() variant that
>> unhashes the inode on match.  Also needs insert_inode_locked4() to use
>> hlist_for_each_entry_safe() instead of hlist_for_each_entry().
>>
>
> Do you really think that this corner use case calls for such actions,
> as creating flavors of inode cache helpers?

Yes, if it simplifies error handling.

> Remember that the so called "offending" inode, is not offending in
> a way that is wrong or incomplete in any way.

Right, so what about just using that inode instead of erroring out?

Thanks,
Miklos

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

On Wed, May 16, 2018 at 1:14 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, May 16, 2018 at 11:51 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Wed, May 16, 2018 at 11:34 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Tue, May 15, 2018 at 3:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>>>>> Currently, there is a small window where ovl_obtain_alias() can
>>>>>> race with ovl_instantiate() and create two different overlay inodes
>>>>>> with the same underlying real non-dir non-hardlink inode.
>>>>>>
>>>>>> The race requires an adversary to guess the file handle of the
>>>>>> yet to be created upper inode and decode the guessed file handle
>>>>>> after ovl_creat_real(), but before ovl_instantiate().
>>>>>>
>>>>>> This patch fixes the race, by using insert_inode_locked4() to add
>>>>>> a newly created inode to icache.
>>>>>>
>>>>>> If the newly created inode apears to already exist in icache (hashed
>>>>>> by the same real upper inode), we export this error to user instead
>>>>>> of silently not hashing the new inode.
>>>>>
>>>>> So we might return an error to user saying operation failed, but still
>>>>> create file on upper. Does that sound little odd?
>>>>>
>>>>
>>>> Yes, but I don't see a better solution.
>>>
>>> Might be better to kick the other, offending inode out, instead of
>>> returning an error.  It would also simplify the error handling.
>>>
>>> We can do that by creating an ovl_inode_test_kick() variant that
>>> unhashes the inode on match.  Also needs insert_inode_locked4() to use
>>> hlist_for_each_entry_safe() instead of hlist_for_each_entry().
>>>
>>
>> Do you really think that this corner use case calls for such actions,
>> as creating flavors of inode cache helpers?
>
> Yes, if it simplifies error handling.
>
>> Remember that the so called "offending" inode, is not offending in
>> a way that is wrong or incomplete in any way.
>
> Right, so what about just using that inode instead of erroring out?
>

OK. I'll see what comes out.

Thanks,
Amir.

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

On Tue, May 15, 2018 at 4:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>> Currently, there is a small window where ovl_obtain_alias() can
>>> race with ovl_instantiate() and create two different overlay inodes
>>> with the same underlying real non-dir non-hardlink inode.
>>>
>>> The race requires an adversary to guess the file handle of the
>>> yet to be created upper inode and decode the guessed file handle
>>> after ovl_creat_real(), but before ovl_instantiate().
>>>
>>> This patch fixes the race, by using insert_inode_locked4() to add
>>> a newly created inode to icache.
>>>
>>> If the newly created inode apears to already exist in icache (hashed
>>> by the same real upper inode), we export this error to user instead
>>> of silently not hashing the new inode.
>>
>> So we might return an error to user saying operation failed, but still
>> create file on upper. Does that sound little odd?
>>
>
> Yes, but I don't see a better solution.
>
>> I am wondering why can't we call ovl_get_inode() in object creation
>> path. That should take care of race between creation path and file
>> handle decode and only one of the paths will get to instantiate and
>> initialize ovl_inode and other path will wait.
>>
>
> I don't even want to think if what you wrote makes sense.
> Remember that the use case we are talking about is quite imaginary.
> Ensuring internal structures consistency in our code and returning
> error to user is the right thing to do for imaginary use cases IMO.
>

Having being forced to think about it ;-), I think using ovl_get_inode()
in create code does make a weird sort of sense.

The reason it is weird is because we will always be throwing away
the new inode that we allocated in ovl_create_object().
AFAICS, if only reason we need to allocate new inode in
ovl_create_object() is to calculate i_mode with inode_init_owner()
and that calculation can be factored out to not need an inode.

The reason it does makes sense to use ovl_get_inode(), is because
the alternative that Miklos suggested is to insert the new inode on
the very very likely use case and throw away the new inode in the
very very unlikely use case of collision. That alternative creates a
somewhat complicated code path that is rarely to never executed -
not the best practice.

Thanks,
Amir.

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Miklos Szeredi]

On Thu, May 17, 2018 at 8:03 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, May 15, 2018 at 4:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>>> Currently, there is a small window where ovl_obtain_alias() can
>>>> race with ovl_instantiate() and create two different overlay inodes
>>>> with the same underlying real non-dir non-hardlink inode.
>>>>
>>>> The race requires an adversary to guess the file handle of the
>>>> yet to be created upper inode and decode the guessed file handle
>>>> after ovl_creat_real(), but before ovl_instantiate().
>>>>
>>>> This patch fixes the race, by using insert_inode_locked4() to add
>>>> a newly created inode to icache.
>>>>
>>>> If the newly created inode apears to already exist in icache (hashed
>>>> by the same real upper inode), we export this error to user instead
>>>> of silently not hashing the new inode.
>>>
>>> So we might return an error to user saying operation failed, but still
>>> create file on upper. Does that sound little odd?
>>>
>>
>> Yes, but I don't see a better solution.
>>
>>> I am wondering why can't we call ovl_get_inode() in object creation
>>> path. That should take care of race between creation path and file
>>> handle decode and only one of the paths will get to instantiate and
>>> initialize ovl_inode and other path will wait.
>>>
>>
>> I don't even want to think if what you wrote makes sense.
>> Remember that the use case we are talking about is quite imaginary.
>> Ensuring internal structures consistency in our code and returning
>> error to user is the right thing to do for imaginary use cases IMO.
>>
>
> Having being forced to think about it ;-), I think using ovl_get_inode()
> in create code does make a weird sort of sense.

Going through the same code-path very much makes sense.

> The reason it is weird is because we will always be throwing away
> the new inode that we allocated in ovl_create_object().
> AFAICS, if only reason we need to allocate new inode in
> ovl_create_object() is to calculate i_mode with inode_init_owner()
> and that calculation can be factored out to not need an inode.

Not the only reason: we don't want inode allocation to fail after
successful creation.  Solution: add a preallocated inode argument to
ovl_get_inode() and deal with allocation failure there.

> The reason it does makes sense to use ovl_get_inode(), is because
> the alternative that Miklos suggested is to insert the new inode on
> the very very likely use case and throw away the new inode in the
> very very unlikely use case of collision. That alternative creates a
> somewhat complicated code path that is rarely to never executed -
> not the best practice.

Agreed completely.

Thanks,
Miklos

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

On Thu, May 17, 2018 at 11:10 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, May 17, 2018 at 8:03 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, May 15, 2018 at 4:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>>>> Currently, there is a small window where ovl_obtain_alias() can
>>>>> race with ovl_instantiate() and create two different overlay inodes
>>>>> with the same underlying real non-dir non-hardlink inode.
>>>>>
>>>>> The race requires an adversary to guess the file handle of the
>>>>> yet to be created upper inode and decode the guessed file handle
>>>>> after ovl_creat_real(), but before ovl_instantiate().
>>>>>
>>>>> This patch fixes the race, by using insert_inode_locked4() to add
>>>>> a newly created inode to icache.
>>>>>
>>>>> If the newly created inode apears to already exist in icache (hashed
>>>>> by the same real upper inode), we export this error to user instead
>>>>> of silently not hashing the new inode.
>>>>
>>>> So we might return an error to user saying operation failed, but still
>>>> create file on upper. Does that sound little odd?
>>>>
>>>
>>> Yes, but I don't see a better solution.
>>>
>>>> I am wondering why can't we call ovl_get_inode() in object creation
>>>> path. That should take care of race between creation path and file
>>>> handle decode and only one of the paths will get to instantiate and
>>>> initialize ovl_inode and other path will wait.
>>>>
>>>
>>> I don't even want to think if what you wrote makes sense.
>>> Remember that the use case we are talking about is quite imaginary.
>>> Ensuring internal structures consistency in our code and returning
>>> error to user is the right thing to do for imaginary use cases IMO.
>>>
>>
>> Having being forced to think about it ;-), I think using ovl_get_inode()
>> in create code does make a weird sort of sense.
>
> Going through the same code-path very much makes sense.
>
>> The reason it is weird is because we will always be throwing away
>> the new inode that we allocated in ovl_create_object().
>> AFAICS, if only reason we need to allocate new inode in
>> ovl_create_object() is to calculate i_mode with inode_init_owner()
>> and that calculation can be factored out to not need an inode.
>
> Not the only reason: we don't want inode allocation to fail after
> successful creation.  Solution: add a preallocated inode argument to
> ovl_get_inode() and deal with allocation failure there.
>

IIUC that would be moving the problem to another place.
Are you suggesting that only in ENOMEM case we resort to
using the preallocated inode and inserting it safely to cache?

I'll make a variant of iget5_locked() that takes a preallocated inode
argument to use instead of alloc_inode() and will always use that
code path for creating objects.

Or maybe that is what you meant.

Thanks,
Amir.

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Miklos Szeredi]

On Thu, May 17, 2018 at 10:10:58AM +0200, Miklos Szeredi wrote:
> 
> Not the only reason: we don't want inode allocation to fail after
> successful creation.  Solution: add a preallocated inode argument to
> ovl_get_inode() and deal with allocation failure there.

Here's a patch to split a helper out of iget5_locked() that takes a preallocated
inode.  It makes the whole thing more readable, IMO, regardless of overlayfs's
needs.

Thanks,
Miklos

---
diff --git a/fs/inode.c b/fs/inode.c
index 13ceb98c3bd3..bb79e3f96147 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1002,6 +1002,52 @@ void unlock_two_nondirectories(struct inode *inode1, struct inode *inode2)
 }
 EXPORT_SYMBOL(unlock_two_nondirectories);
 
+struct inode *iget5_prealloc(struct inode *inode,
+			     struct super_block *sb, unsigned long hashval,
+			     int (*test)(struct inode *, void *),
+			     int (*set)(struct inode *, void *), void *data)
+{
+	struct hlist_head *head = inode_hashtable + hash(sb, hashval);
+	struct inode *old;
+
+again:
+	spin_lock(&inode_hash_lock);
+	old = find_inode(sb, head, test, data);
+	if (unlikely(old)) {
+		/*
+		 * Uhhuh, somebody else created the same inode under us.
+		 * Use the old inode instead of the preallocated one.
+		 */
+		spin_unlock(&inode_hash_lock);
+		wait_on_inode(old);
+		if (unlikely(inode_unhashed(old))) {
+			iput(old);
+			goto again;
+		}
+		return old;
+	}
+
+	if (unlikely(set(inode, data))) {
+		inode = NULL;
+		goto unlock;
+	}
+
+	/*
+	 * Return the locked inode with I_NEW set, the
+	 * caller is responsible for filling in the contents
+	 */
+	spin_lock(&inode->i_lock);
+	inode->i_state = I_NEW;
+	hlist_add_head(&inode->i_hash, head);
+	spin_unlock(&inode->i_lock);
+	inode_sb_list_add(inode);
+unlock:
+	spin_unlock(&inode_hash_lock);
+
+	return inode;
+}
+EXPORT_SYMBOL(iget5_prealloc);
+
 /**
  * iget5_locked - obtain an inode from a mounted file system
  * @sb:		super block of file system
@@ -1026,66 +1072,18 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
 		int (*test)(struct inode *, void *),
 		int (*set)(struct inode *, void *), void *data)
 {
-	struct hlist_head *head = inode_hashtable + hash(sb, hashval);
-	struct inode *inode;
-again:
-	spin_lock(&inode_hash_lock);
-	inode = find_inode(sb, head, test, data);
-	spin_unlock(&inode_hash_lock);
+	struct inode *inode = ilookup5(sb, hashval, test, data);
 
-	if (inode) {
-		wait_on_inode(inode);
-		if (unlikely(inode_unhashed(inode))) {
-			iput(inode);
-			goto again;
-		}
-		return inode;
-	}
-
-	inode = alloc_inode(sb);
-	if (inode) {
-		struct inode *old;
-
-		spin_lock(&inode_hash_lock);
-		/* We released the lock, so.. */
-		old = find_inode(sb, head, test, data);
-		if (!old) {
-			if (set(inode, data))
-				goto set_failed;
-
-			spin_lock(&inode->i_lock);
-			inode->i_state = I_NEW;
-			hlist_add_head(&inode->i_hash, head);
-			spin_unlock(&inode->i_lock);
-			inode_sb_list_add(inode);
-			spin_unlock(&inode_hash_lock);
-
-			/* Return the locked inode with I_NEW set, the
-			 * caller is responsible for filling in the contents
-			 */
-			return inode;
-		}
+	if (!inode) {
+		struct inode *new = alloc_inode(sb);
 
-		/*
-		 * Uhhuh, somebody else created the same inode under
-		 * us. Use the old inode instead of the one we just
-		 * allocated.
-		 */
-		spin_unlock(&inode_hash_lock);
-		destroy_inode(inode);
-		inode = old;
-		wait_on_inode(inode);
-		if (unlikely(inode_unhashed(inode))) {
-			iput(inode);
-			goto again;
+		if (new) {
+			inode = iget5_prealloc(new, sb, hashval, test, set, data);
+			if (inode != new)
+				destroy_inode(new);
 		}
 	}
 	return inode;
-
-set_failed:
-	spin_unlock(&inode_hash_lock);
-	destroy_inode(inode);
-	return NULL;
 }
 EXPORT_SYMBOL(iget5_locked);
 

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

On Thu, May 17, 2018 at 11:53 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, May 17, 2018 at 10:10:58AM +0200, Miklos Szeredi wrote:
>>
>> Not the only reason: we don't want inode allocation to fail after
>> successful creation.  Solution: add a preallocated inode argument to
>> ovl_get_inode() and deal with allocation failure there.
>
> Here's a patch to split a helper out of iget5_locked() that takes a preallocated
> inode.  It makes the whole thing more readable, IMO, regardless of overlayfs's
> needs.
>

That looks good. I'll use that.
One suggestion below.

Thanks,
Amir.

>
> ---
> diff --git a/fs/inode.c b/fs/inode.c
> index 13ceb98c3bd3..bb79e3f96147 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1002,6 +1002,52 @@ void unlock_two_nondirectories(struct inode *inode1, struct inode *inode2)
>  }
>  EXPORT_SYMBOL(unlock_two_nondirectories);
>
> +struct inode *iget5_prealloc(struct inode *inode,
> +                            struct super_block *sb, unsigned long hashval,

Maybe no need to pass in @sb...

> +                            int (*test)(struct inode *, void *),
> +                            int (*set)(struct inode *, void *), void *data)
> +{
> +       struct hlist_head *head = inode_hashtable + hash(sb, hashval);
> +       struct inode *old;
> +
> +again:
> +       spin_lock(&inode_hash_lock);
> +       old = find_inode(sb, head, test, data);
> +       if (unlikely(old)) {
> +               /*
> +                * Uhhuh, somebody else created the same inode under us.
> +                * Use the old inode instead of the preallocated one.
> +                */
> +               spin_unlock(&inode_hash_lock);
> +               wait_on_inode(old);
> +               if (unlikely(inode_unhashed(old))) {
> +                       iput(old);
> +                       goto again;
> +               }
> +               return old;
> +       }
> +
> +       if (unlikely(set(inode, data))) {
> +               inode = NULL;
> +               goto unlock;
> +       }
> +
> +       /*
> +        * Return the locked inode with I_NEW set, the
> +        * caller is responsible for filling in the contents
> +        */
> +       spin_lock(&inode->i_lock);
> +       inode->i_state = I_NEW;
> +       hlist_add_head(&inode->i_hash, head);
> +       spin_unlock(&inode->i_lock);
> +       inode_sb_list_add(inode);
> +unlock:
> +       spin_unlock(&inode_hash_lock);
> +
> +       return inode;
> +}
> +EXPORT_SYMBOL(iget5_prealloc);
> +
>  /**
>   * iget5_locked - obtain an inode from a mounted file system
>   * @sb:                super block of file system
> @@ -1026,66 +1072,18 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
>                 int (*test)(struct inode *, void *),
>                 int (*set)(struct inode *, void *), void *data)
>  {
> -       struct hlist_head *head = inode_hashtable + hash(sb, hashval);
> -       struct inode *inode;
> -again:
> -       spin_lock(&inode_hash_lock);
> -       inode = find_inode(sb, head, test, data);
> -       spin_unlock(&inode_hash_lock);
> +       struct inode *inode = ilookup5(sb, hashval, test, data);
>
> -       if (inode) {
> -               wait_on_inode(inode);
> -               if (unlikely(inode_unhashed(inode))) {
> -                       iput(inode);
> -                       goto again;
> -               }
> -               return inode;
> -       }
> -
> -       inode = alloc_inode(sb);
> -       if (inode) {
> -               struct inode *old;
> -
> -               spin_lock(&inode_hash_lock);
> -               /* We released the lock, so.. */
> -               old = find_inode(sb, head, test, data);
> -               if (!old) {
> -                       if (set(inode, data))
> -                               goto set_failed;
> -
> -                       spin_lock(&inode->i_lock);
> -                       inode->i_state = I_NEW;
> -                       hlist_add_head(&inode->i_hash, head);
> -                       spin_unlock(&inode->i_lock);
> -                       inode_sb_list_add(inode);
> -                       spin_unlock(&inode_hash_lock);
> -
> -                       /* Return the locked inode with I_NEW set, the
> -                        * caller is responsible for filling in the contents
> -                        */
> -                       return inode;
> -               }
> +       if (!inode) {
> +               struct inode *new = alloc_inode(sb);
>
> -               /*
> -                * Uhhuh, somebody else created the same inode under
> -                * us. Use the old inode instead of the one we just
> -                * allocated.
> -                */
> -               spin_unlock(&inode_hash_lock);
> -               destroy_inode(inode);
> -               inode = old;
> -               wait_on_inode(inode);
> -               if (unlikely(inode_unhashed(inode))) {
> -                       iput(inode);
> -                       goto again;
> +               if (new) {
> +                       inode = iget5_prealloc(new, sb, hashval, test, set, data);
> +                       if (inode != new)
> +                               destroy_inode(new);
>                 }
>         }
>         return inode;
> -
> -set_failed:
> -       spin_unlock(&inode_hash_lock);
> -       destroy_inode(inode);
> -       return NULL;
>  }
>  EXPORT_SYMBOL(iget5_locked);
>

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Miklos Szeredi]

On Thu, May 17, 2018 at 10:58 AM, Amir Goldstein <amir73il@gmail.com> wrote:

>> ---
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 13ceb98c3bd3..bb79e3f96147 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -1002,6 +1002,52 @@ void unlock_two_nondirectories(struct inode *inode1, struct inode *inode2)
>>  }
>>  EXPORT_SYMBOL(unlock_two_nondirectories);
>>
>> +struct inode *iget5_prealloc(struct inode *inode,
>> +                            struct super_block *sb, unsigned long hashval,
>
> Maybe no need to pass in @sb...

Yep.

Thanks,
Miklos

Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein]

On Thu, May 17, 2018 at 12:07 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, May 17, 2018 at 10:58 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>
>>> ---
>>> diff --git a/fs/inode.c b/fs/inode.c
>>> index 13ceb98c3bd3..bb79e3f96147 100644
>>> --- a/fs/inode.c
>>> +++ b/fs/inode.c
>>> @@ -1002,6 +1002,52 @@ void unlock_two_nondirectories(struct inode *inode1, struct inode *inode2)
>>>  }
>>>  EXPORT_SYMBOL(unlock_two_nondirectories);
>>>
>>> +struct inode *iget5_prealloc(struct inode *inode,
>>> +                            struct super_block *sb, unsigned long hashval,
>>
>> Maybe no need to pass in @sb...
>

Pushed new version with iget5_prealloc() and another cleanup patch
from Vivek to:
https://github.com/amir73il/linux/commits/ovl-fixes

It passed xfstest overlay/quick.

Will run some more tests tomorrow and post the patches.

Thanks,
Amir.

[PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch

[Amir Goldstein]

Overlayfs should cope with online changes to underlying layer
without crashing the kernel, which is what xfstest overlay/019
checks.

This test may sometimes trigger WARN_ON() in ovl_create_or_link()
when linking an overlay inode that has been changed on underlying
layer.

Replace those WARN_ON() with pr_warn_ratelimited() to prevent
test from failing and because this is more appropriate to the
use case.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 62e6733b755c..25b339278684 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 	if (!err) {
 		struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
 
-		WARN_ON(inode->i_mode != realinode->i_mode);
-		WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
-		WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
+		if (inode->i_mode != realinode->i_mode ||
+		    !uid_eq(inode->i_uid, realinode->i_uid) ||
+		    !gid_eq(inode->i_gid, realinode->i_gid)) {
+			pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n",
+				dentry, inode->i_mode,
+				from_kuid(&init_user_ns, inode->i_uid),
+				from_kgid(&init_user_ns, inode->i_gid),
+				realinode->i_mode,
+				from_kuid(&init_user_ns, realinode->i_uid),
+				from_kgid(&init_user_ns, realinode->i_gid));
+		}
 	}
 	return err;
 }
-- 
2.7.4

Re: [PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch

[Vivek Goyal]

On Tue, May 15, 2018 at 01:26:10PM +0300, Amir Goldstein wrote:
> Overlayfs should cope with online changes to underlying layer
> without crashing the kernel, which is what xfstest overlay/019
> checks.
> 
> This test may sometimes trigger WARN_ON() in ovl_create_or_link()
> when linking an overlay inode that has been changed on underlying
> layer.
> 
> Replace those WARN_ON() with pr_warn_ratelimited() to prevent
> test from failing and because this is more appropriate to the
> use case.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/dir.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 62e6733b755c..25b339278684 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>  	if (!err) {
>  		struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
>  
> -		WARN_ON(inode->i_mode != realinode->i_mode);
> -		WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
> -		WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
> +		if (inode->i_mode != realinode->i_mode ||
> +		    !uid_eq(inode->i_uid, realinode->i_uid) ||
> +		    !gid_eq(inode->i_gid, realinode->i_gid)) {
> +			pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n",
> +				dentry, inode->i_mode,
> +				from_kuid(&init_user_ns, inode->i_uid),
> +				from_kgid(&init_user_ns, inode->i_gid),
> +				realinode->i_mode,
> +				from_kuid(&init_user_ns, realinode->i_uid),
> +				from_kgid(&init_user_ns, realinode->i_gid));

Curious that why these calls to from_kuid() and from_kgid() was required
in this patch.

Vivek

Re: [PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch

[Amir Goldstein]

On Tue, May 15, 2018 at 3:48 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, May 15, 2018 at 01:26:10PM +0300, Amir Goldstein wrote:
>> Overlayfs should cope with online changes to underlying layer
>> without crashing the kernel, which is what xfstest overlay/019
>> checks.
>>
>> This test may sometimes trigger WARN_ON() in ovl_create_or_link()
>> when linking an overlay inode that has been changed on underlying
>> layer.
>>
>> Replace those WARN_ON() with pr_warn_ratelimited() to prevent
>> test from failing and because this is more appropriate to the
>> use case.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/dir.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index 62e6733b755c..25b339278684 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>>       if (!err) {
>>               struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
>>
>> -             WARN_ON(inode->i_mode != realinode->i_mode);
>> -             WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
>> -             WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
>> +             if (inode->i_mode != realinode->i_mode ||
>> +                 !uid_eq(inode->i_uid, realinode->i_uid) ||
>> +                 !gid_eq(inode->i_gid, realinode->i_gid)) {
>> +                     pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n",
>> +                             dentry, inode->i_mode,
>> +                             from_kuid(&init_user_ns, inode->i_uid),
>> +                             from_kgid(&init_user_ns, inode->i_gid),
>> +                             realinode->i_mode,
>> +                             from_kuid(&init_user_ns, realinode->i_uid),
>> +                             from_kgid(&init_user_ns, realinode->i_gid));
>
> Curious that why these calls to from_kuid() and from_kgid() was required
> in this patch.
>

kuid/kgid are opaque structs we should not access directly, so
I thought this was the standard way to get a value for printing.
Maybe I was wrong.

Thanks,
Amir.

Re: [PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch

[Miklos Szeredi]

On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Overlayfs should cope with online changes to underlying layer
> without crashing the kernel, which is what xfstest overlay/019
> checks.
>
> This test may sometimes trigger WARN_ON() in ovl_create_or_link()
> when linking an overlay inode that has been changed on underlying
> layer.
>
> Replace those WARN_ON() with pr_warn_ratelimited() to prevent
> test from failing and because this is more appropriate to the
> use case.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/dir.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 62e6733b755c..25b339278684 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>         if (!err) {
>                 struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
>
> -               WARN_ON(inode->i_mode != realinode->i_mode);
> -               WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
> -               WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
> +               if (inode->i_mode != realinode->i_mode ||
> +                   !uid_eq(inode->i_uid, realinode->i_uid) ||
> +                   !gid_eq(inode->i_gid, realinode->i_gid)) {
> +                       pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n",
> +                               dentry, inode->i_mode,
> +                               from_kuid(&init_user_ns, inode->i_uid),
> +                               from_kgid(&init_user_ns, inode->i_gid),
> +                               realinode->i_mode,
> +                               from_kuid(&init_user_ns, realinode->i_uid),
> +                               from_kgid(&init_user_ns, realinode->i_gid));
> +               }

How about just dropping the warnings altogether.  They didn't discover
an issue with the code, just something normal, so IMO we should just
get rid of them.

Thanks,
Miklois

Re: [PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch

[Amir Goldstein]

On Wed, May 16, 2018 at 1:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Overlayfs should cope with online changes to underlying layer
>> without crashing the kernel, which is what xfstest overlay/019
>> checks.
>>
>> This test may sometimes trigger WARN_ON() in ovl_create_or_link()
>> when linking an overlay inode that has been changed on underlying
>> layer.
>>
>> Replace those WARN_ON() with pr_warn_ratelimited() to prevent
>> test from failing and because this is more appropriate to the
>> use case.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/dir.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index 62e6733b755c..25b339278684 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>>         if (!err) {
>>                 struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
>>
>> -               WARN_ON(inode->i_mode != realinode->i_mode);
>> -               WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
>> -               WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
>> +               if (inode->i_mode != realinode->i_mode ||
>> +                   !uid_eq(inode->i_uid, realinode->i_uid) ||
>> +                   !gid_eq(inode->i_gid, realinode->i_gid)) {
>> +                       pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n",
>> +                               dentry, inode->i_mode,
>> +                               from_kuid(&init_user_ns, inode->i_uid),
>> +                               from_kgid(&init_user_ns, inode->i_gid),
>> +                               realinode->i_mode,
>> +                               from_kuid(&init_user_ns, realinode->i_uid),
>> +                               from_kgid(&init_user_ns, realinode->i_gid));
>> +               }
>
> How about just dropping the warnings altogether.  They didn't discover
> an issue with the code, just something normal, so IMO we should just
> get rid of them.
>

OK. On that subject, do you want to leave the 'debug' argument
to ovl_do_XXX? I started peeling it off slowly from the new helpers,
but maybe we should just yank it completely from the ovl_do_XXX
helpers? pr_debug can be disabled dynamically anyway. right?

Thanks,
Amir.

Re: [PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch

[Miklos Szeredi]

On Wed, May 16, 2018 at 1:06 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, May 16, 2018 at 1:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> Overlayfs should cope with online changes to underlying layer
>>> without crashing the kernel, which is what xfstest overlay/019
>>> checks.
>>>
>>> This test may sometimes trigger WARN_ON() in ovl_create_or_link()
>>> when linking an overlay inode that has been changed on underlying
>>> layer.
>>>
>>> Replace those WARN_ON() with pr_warn_ratelimited() to prevent
>>> test from failing and because this is more appropriate to the
>>> use case.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>  fs/overlayfs/dir.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>>> index 62e6733b755c..25b339278684 100644
>>> --- a/fs/overlayfs/dir.c
>>> +++ b/fs/overlayfs/dir.c
>>> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>>>         if (!err) {
>>>                 struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
>>>
>>> -               WARN_ON(inode->i_mode != realinode->i_mode);
>>> -               WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
>>> -               WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
>>> +               if (inode->i_mode != realinode->i_mode ||
>>> +                   !uid_eq(inode->i_uid, realinode->i_uid) ||
>>> +                   !gid_eq(inode->i_gid, realinode->i_gid)) {
>>> +                       pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n",
>>> +                               dentry, inode->i_mode,
>>> +                               from_kuid(&init_user_ns, inode->i_uid),
>>> +                               from_kgid(&init_user_ns, inode->i_gid),
>>> +                               realinode->i_mode,
>>> +                               from_kuid(&init_user_ns, realinode->i_uid),
>>> +                               from_kgid(&init_user_ns, realinode->i_gid));
>>> +               }
>>
>> How about just dropping the warnings altogether.  They didn't discover
>> an issue with the code, just something normal, so IMO we should just
>> get rid of them.
>>
>
> OK. On that subject, do you want to leave the 'debug' argument
> to ovl_do_XXX? I started peeling it off slowly from the new helpers,
> but maybe we should just yank it completely from the ovl_do_XXX
> helpers? pr_debug can be disabled dynamically anyway. right?

Right.  The original idea was to not debug upper operations that are
just verbatim copies of the overlay operation, but I guess it doesn't
really hurt to debug unconditionally.

Thanks,
Miklos

Re: [PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch

[Amir Goldstein]

On Wed, May 16, 2018 at 2:18 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, May 16, 2018 at 1:06 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Wed, May 16, 2018 at 1:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> Overlayfs should cope with online changes to underlying layer
>>>> without crashing the kernel, which is what xfstest overlay/019
>>>> checks.
>>>>
>>>> This test may sometimes trigger WARN_ON() in ovl_create_or_link()
>>>> when linking an overlay inode that has been changed on underlying
>>>> layer.
>>>>
>>>> Replace those WARN_ON() with pr_warn_ratelimited() to prevent
>>>> test from failing and because this is more appropriate to the
>>>> use case.
>>>>
>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>>> ---
>>>>  fs/overlayfs/dir.c | 14 +++++++++++---
>>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>>>> index 62e6733b755c..25b339278684 100644
>>>> --- a/fs/overlayfs/dir.c
>>>> +++ b/fs/overlayfs/dir.c
>>>> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>>>>         if (!err) {
>>>>                 struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
>>>>
>>>> -               WARN_ON(inode->i_mode != realinode->i_mode);
>>>> -               WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
>>>> -               WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
>>>> +               if (inode->i_mode != realinode->i_mode ||
>>>> +                   !uid_eq(inode->i_uid, realinode->i_uid) ||
>>>> +                   !gid_eq(inode->i_gid, realinode->i_gid)) {
>>>> +                       pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n",
>>>> +                               dentry, inode->i_mode,
>>>> +                               from_kuid(&init_user_ns, inode->i_uid),
>>>> +                               from_kgid(&init_user_ns, inode->i_gid),
>>>> +                               realinode->i_mode,
>>>> +                               from_kuid(&init_user_ns, realinode->i_uid),
>>>> +                               from_kgid(&init_user_ns, realinode->i_gid));
>>>> +               }
>>>
>>> How about just dropping the warnings altogether.  They didn't discover
>>> an issue with the code, just something normal, so IMO we should just
>>> get rid of them.
>>>
>>

I guess it would be wise to leave that check in at least for the case we end
up using a cached inode instead of the new inode we created...

Thanks,
Amir.

[PATCH v3 3/4] ovl: create helper ovl_create_temp()

[Amir Goldstein]

Al Viro suggested to simplify callers of ovl_create_real() by
returning the created dentry (or ERR_PTR) from ovl_create_real().
This is a spin off of his suggestion, which is cleaner in my opinion.

Also created a wrapper ovl_create_temp_dir() and used it in
ovl_create_index() instead of calling ovl_do_mkdir(), so now all callers
of ovl_do_mkdir() are routed through ovl_create_real(), which paves the
way for Al's fix for non-hashed result from vfs_mkdir().

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 27 +++++++--------------------
 fs/overlayfs/dir.c       | 47 +++++++++++++++++++++++++++++------------------
 fs/overlayfs/overlayfs.h |  9 ++++++++-
 3 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8bede0742619..4ba16cbeaec2 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -365,14 +365,10 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
 	if (err)
 		return err;
 
-	temp = ovl_lookup_temp(indexdir);
+	temp = ovl_create_temp_dir(indexdir);
 	if (IS_ERR(temp))
 		goto temp_err;
 
-	err = ovl_do_mkdir(dir, temp, S_IFDIR, true);
-	if (err)
-		goto out;
-
 	err = ovl_set_upper_fh(upper, temp);
 	if (err)
 		goto out_cleanup;
@@ -501,22 +497,13 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 	if (new_creds)
 		old_creds = override_creds(new_creds);
 
-	if (c->tmpfile) {
+	if (c->tmpfile)
 		temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
-		if (IS_ERR(temp))
-			goto temp_err;
-	} else {
-		temp = ovl_lookup_temp(c->workdir);
-		if (IS_ERR(temp))
-			goto temp_err;
-
-		err = ovl_create_real(d_inode(c->workdir), temp, &cattr,
-				      NULL, true);
-		if (err) {
-			dput(temp);
-			goto out;
-		}
-	}
+	else
+		temp = ovl_create_temp(c->workdir, &cattr, NULL);
+	if (IS_ERR(temp))
+		goto temp_err;
+
 	err = 0;
 	*tempp = temp;
 out:
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 25b339278684..45f5f9232e71 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -160,6 +160,26 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 	return err;
 }
 
+struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
+			       struct dentry *hardlink)
+{
+	struct inode *wdir = workdir->d_inode;
+	struct dentry *temp;
+	int err;
+
+	temp = ovl_lookup_temp(workdir);
+	if (IS_ERR(temp))
+		return temp;
+
+	err = ovl_create_real(wdir, temp, attr, hardlink, true);
+	if (err) {
+		dput(temp);
+		return ERR_PTR(err);
+	}
+
+	return temp;
+}
+
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
 			       int xerr)
 {
@@ -292,15 +312,11 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (upper->d_parent->d_inode != udir)
 		goto out_unlock;
 
-	opaquedir = ovl_lookup_temp(workdir);
+	opaquedir = ovl_create_temp_dir(workdir);
 	err = PTR_ERR(opaquedir);
 	if (IS_ERR(opaquedir))
-		goto out_unlock;
-
-	err = ovl_create_real(wdir, opaquedir,
-			      &(struct cattr){.mode = stat.mode}, NULL, true);
 	if (err)
-		goto out_dput;
+		goto out_unlock;
 
 	err = ovl_copy_xattr(upper, opaquedir);
 	if (err)
@@ -331,7 +347,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 
 out_cleanup:
 	ovl_cleanup(wdir, opaquedir);
-out_dput:
 	dput(opaquedir);
 out_unlock:
 	unlock_rename(workdir, upperdir);
@@ -392,20 +407,16 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	if (err)
 		goto out;
 
-	newdentry = ovl_lookup_temp(workdir);
-	err = PTR_ERR(newdentry);
-	if (IS_ERR(newdentry))
-		goto out_unlock;
-
 	upper = lookup_one_len(dentry->d_name.name, upperdir,
 			       dentry->d_name.len);
 	err = PTR_ERR(upper);
 	if (IS_ERR(upper))
-		goto out_dput;
+		goto out_unlock;
 
-	err = ovl_create_real(wdir, newdentry, cattr, hardlink, true);
-	if (err)
-		goto out_dput2;
+	newdentry = ovl_create_temp(workdir, cattr, hardlink);
+	err = PTR_ERR(newdentry);
+	if (IS_ERR(newdentry))
+		goto out_dput;
 
 	/*
 	 * mode could have been mutilated due to umask (e.g. sgid directory)
@@ -454,9 +465,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
 	newdentry = NULL;
 out_dput2:
-	dput(upper);
-out_dput:
 	dput(newdentry);
+out_dput:
+	dput(upper);
 out_unlock:
 	unlock_rename(workdir, upperdir);
 out:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 642b25702092..967183175ef5 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -369,6 +369,7 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
 
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
+int ovl_cleanup(struct inode *dir, struct dentry *dentry);
 struct dentry *ovl_lookup_temp(struct dentry *workdir);
 int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 			     struct dentry *dentry);
@@ -380,7 +381,13 @@ struct cattr {
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct cattr *attr,
 		    struct dentry *hardlink, bool debug);
-int ovl_cleanup(struct inode *dir, struct dentry *dentry);
+struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
+			       struct dentry *hardlink);
+
+static inline struct dentry *ovl_create_temp_dir(struct dentry *workdir)
+{
+	return ovl_create_temp(workdir, &(struct cattr){.mode = S_IFDIR}, NULL);
+}
 
 /* file.c */
 extern const struct file_operations ovl_file_operations;
-- 
2.7.4

Re: [PATCH v3 3/4] ovl: create helper ovl_create_temp()

[Miklos Szeredi]

On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Al Viro suggested to simplify callers of ovl_create_real() by
> returning the created dentry (or ERR_PTR) from ovl_create_real().
> This is a spin off of his suggestion, which is cleaner in my opinion.
>
> Also created a wrapper ovl_create_temp_dir() and used it in
> ovl_create_index() instead of calling ovl_do_mkdir(), so now all callers
> of ovl_do_mkdir() are routed through ovl_create_real(), which paves the
> way for Al's fix for non-hashed result from vfs_mkdir().
>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/copy_up.c   | 27 +++++++--------------------
>  fs/overlayfs/dir.c       | 47 +++++++++++++++++++++++++++++------------------
>  fs/overlayfs/overlayfs.h |  9 ++++++++-
>  3 files changed, 44 insertions(+), 39 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8bede0742619..4ba16cbeaec2 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -365,14 +365,10 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
>         if (err)
>                 return err;
>
> -       temp = ovl_lookup_temp(indexdir);
> +       temp = ovl_create_temp_dir(indexdir);

#define OVL_DIR_CATTR(m) (&(struct cattr) { .mode = S_IFDIR | (m) })
        temp = ovl_create_temp(indexdir, OVL_DIR_CATTR(0), NULL);

>         if (IS_ERR(temp))
>                 goto temp_err;
>
> -       err = ovl_do_mkdir(dir, temp, S_IFDIR, true);
> -       if (err)
> -               goto out;
> -
>         err = ovl_set_upper_fh(upper, temp);
>         if (err)
>                 goto out_cleanup;
> @@ -501,22 +497,13 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
>         if (new_creds)
>                 old_creds = override_creds(new_creds);
>
> -       if (c->tmpfile) {
> +       if (c->tmpfile)
>                 temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
> -               if (IS_ERR(temp))
> -                       goto temp_err;
> -       } else {
> -               temp = ovl_lookup_temp(c->workdir);
> -               if (IS_ERR(temp))
> -                       goto temp_err;
> -
> -               err = ovl_create_real(d_inode(c->workdir), temp, &cattr,
> -                                     NULL, true);
> -               if (err) {
> -                       dput(temp);
> -                       goto out;
> -               }
> -       }
> +       else
> +               temp = ovl_create_temp(c->workdir, &cattr, NULL);
> +       if (IS_ERR(temp))
> +               goto temp_err;
> +
>         err = 0;
>         *tempp = temp;
>  out:
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 25b339278684..45f5f9232e71 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -160,6 +160,26 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>         return err;
>  }
>
> +struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
> +                              struct dentry *hardlink)

Talking of cleanups, can we put hardlink into cattr as well?  (separate patch)

> +{
> +       struct inode *wdir = workdir->d_inode;
> +       struct dentry *temp;
> +       int err;
> +
> +       temp = ovl_lookup_temp(workdir);
> +       if (IS_ERR(temp))
> +               return temp;

What's wrong with viro's version of  dentry in ovl_create_real()?
Turns this whole function into:

        return ovl_create_real(d_inode(workdir),
ovl_lookup_temp(workdir), attr, hardlink, true);

> +
> +       err = ovl_create_real(wdir, temp, attr, hardlink, true);
> +       if (err) {
> +               dput(temp);
> +               return ERR_PTR(err);
> +       }
> +
> +       return temp;
> +}
> +
>  static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
>                                int xerr)
>  {
> @@ -292,15 +312,11 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>         if (upper->d_parent->d_inode != udir)
>                 goto out_unlock;
>
> -       opaquedir = ovl_lookup_temp(workdir);
> +       opaquedir = ovl_create_temp_dir(workdir);

Where has the mode gone?

>         err = PTR_ERR(opaquedir);
>         if (IS_ERR(opaquedir))
> -               goto out_unlock;
> -
> -       err = ovl_create_real(wdir, opaquedir,
> -                             &(struct cattr){.mode = stat.mode}, NULL, true);
>         if (err)
> -               goto out_dput;
> +               goto out_unlock;
>
>         err = ovl_copy_xattr(upper, opaquedir);
>         if (err)
> @@ -331,7 +347,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>
>  out_cleanup:
>         ovl_cleanup(wdir, opaquedir);
> -out_dput:
>         dput(opaquedir);
>  out_unlock:
>         unlock_rename(workdir, upperdir);
> @@ -392,20 +407,16 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>         if (err)
>                 goto out;
>
> -       newdentry = ovl_lookup_temp(workdir);
> -       err = PTR_ERR(newdentry);
> -       if (IS_ERR(newdentry))
> -               goto out_unlock;
> -
>         upper = lookup_one_len(dentry->d_name.name, upperdir,
>                                dentry->d_name.len);
>         err = PTR_ERR(upper);
>         if (IS_ERR(upper))
> -               goto out_dput;
> +               goto out_unlock;
>
> -       err = ovl_create_real(wdir, newdentry, cattr, hardlink, true);
> -       if (err)
> -               goto out_dput2;
> +       newdentry = ovl_create_temp(workdir, cattr, hardlink);
> +       err = PTR_ERR(newdentry);
> +       if (IS_ERR(newdentry))
> +               goto out_dput;
>
>         /*
>          * mode could have been mutilated due to umask (e.g. sgid directory)
> @@ -454,9 +465,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>         err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
>         newdentry = NULL;
>  out_dput2:
> -       dput(upper);
> -out_dput:
>         dput(newdentry);
> +out_dput:
> +       dput(upper);
>  out_unlock:
>         unlock_rename(workdir, upperdir);
>  out:
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 642b25702092..967183175ef5 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -369,6 +369,7 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
>
>  /* dir.c */
>  extern const struct inode_operations ovl_dir_inode_operations;
> +int ovl_cleanup(struct inode *dir, struct dentry *dentry);
>  struct dentry *ovl_lookup_temp(struct dentry *workdir);
>  int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
>                              struct dentry *dentry);
> @@ -380,7 +381,13 @@ struct cattr {
>  int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>                     struct cattr *attr,
>                     struct dentry *hardlink, bool debug);
> -int ovl_cleanup(struct inode *dir, struct dentry *dentry);
> +struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
> +                              struct dentry *hardlink);
> +
> +static inline struct dentry *ovl_create_temp_dir(struct dentry *workdir)
> +{
> +       return ovl_create_temp(workdir, &(struct cattr){.mode = S_IFDIR}, NULL);
> +}
>
>  /* file.c */
>  extern const struct file_operations ovl_file_operations;
> --
> 2.7.4
>

Re: [PATCH v3 3/4] ovl: create helper ovl_create_temp()

[Amir Goldstein]

On Wed, May 16, 2018 at 1:41 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Al Viro suggested to simplify callers of ovl_create_real() by
>> returning the created dentry (or ERR_PTR) from ovl_create_real().
>> This is a spin off of his suggestion, which is cleaner in my opinion.
>>
>> Also created a wrapper ovl_create_temp_dir() and used it in
>> ovl_create_index() instead of calling ovl_do_mkdir(), so now all callers
>> of ovl_do_mkdir() are routed through ovl_create_real(), which paves the
>> way for Al's fix for non-hashed result from vfs_mkdir().
>>
>> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/copy_up.c   | 27 +++++++--------------------
>>  fs/overlayfs/dir.c       | 47 +++++++++++++++++++++++++++++------------------
>>  fs/overlayfs/overlayfs.h |  9 ++++++++-
>>  3 files changed, 44 insertions(+), 39 deletions(-)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index 8bede0742619..4ba16cbeaec2 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -365,14 +365,10 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
>>         if (err)
>>                 return err;
>>
>> -       temp = ovl_lookup_temp(indexdir);
>> +       temp = ovl_create_temp_dir(indexdir);
>
> #define OVL_DIR_CATTR(m) (&(struct cattr) { .mode = S_IFDIR | (m) })
>         temp = ovl_create_temp(indexdir, OVL_DIR_CATTR(0), NULL);
>

OK.

>>         if (IS_ERR(temp))
>>                 goto temp_err;
>>
>> -       err = ovl_do_mkdir(dir, temp, S_IFDIR, true);
>> -       if (err)
>> -               goto out;
>> -
>>         err = ovl_set_upper_fh(upper, temp);
>>         if (err)
>>                 goto out_cleanup;
>> @@ -501,22 +497,13 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
>>         if (new_creds)
>>                 old_creds = override_creds(new_creds);
>>
>> -       if (c->tmpfile) {
>> +       if (c->tmpfile)
>>                 temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
>> -               if (IS_ERR(temp))
>> -                       goto temp_err;
>> -       } else {
>> -               temp = ovl_lookup_temp(c->workdir);
>> -               if (IS_ERR(temp))
>> -                       goto temp_err;
>> -
>> -               err = ovl_create_real(d_inode(c->workdir), temp, &cattr,
>> -                                     NULL, true);
>> -               if (err) {
>> -                       dput(temp);
>> -                       goto out;
>> -               }
>> -       }
>> +       else
>> +               temp = ovl_create_temp(c->workdir, &cattr, NULL);
>> +       if (IS_ERR(temp))
>> +               goto temp_err;
>> +
>>         err = 0;
>>         *tempp = temp;
>>  out:
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index 25b339278684..45f5f9232e71 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -160,6 +160,26 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>>         return err;
>>  }
>>
>> +struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
>> +                              struct dentry *hardlink)
>
> Talking of cleanups, can we put hardlink into cattr as well?  (separate patch)
>

OK. If you don't mind, I'll use this opportunity to also namespace
it to ovl_cattr.

>> +{
>> +       struct inode *wdir = workdir->d_inode;
>> +       struct dentry *temp;
>> +       int err;
>> +
>> +       temp = ovl_lookup_temp(workdir);
>> +       if (IS_ERR(temp))
>> +               return temp;
>
> What's wrong with viro's version of  dentry in ovl_create_real()?
> Turns this whole function into:
>
>         return ovl_create_real(d_inode(workdir),
> ovl_lookup_temp(workdir), attr, hardlink, true);

Nothing. A matter of taste. If you like Al's version better,
I'll add it back in the next patch.

>
>> +
>> +       err = ovl_create_real(wdir, temp, attr, hardlink, true);
>> +       if (err) {
>> +               dput(temp);
>> +               return ERR_PTR(err);
>> +       }
>> +
>> +       return temp;
>> +}
>> +
>>  static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
>>                                int xerr)
>>  {
>> @@ -292,15 +312,11 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>>         if (upper->d_parent->d_inode != udir)
>>                 goto out_unlock;
>>
>> -       opaquedir = ovl_lookup_temp(workdir);
>> +       opaquedir = ovl_create_temp_dir(workdir);
>
> Where has the mode gone?

OOPS. I'll get it back.

Thanks,
Amir.

Re: [PATCH v3 3/4] ovl: create helper ovl_create_temp()

[Miklos Szeredi]

On Wed, May 16, 2018 at 1:15 PM, Amir Goldstein <amir73il@gmail.com> wrote:

>>> @@ -160,6 +160,26 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>>>         return err;
>>>  }
>>>
>>> +struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
>>> +                              struct dentry *hardlink)
>>
>> Talking of cleanups, can we put hardlink into cattr as well?  (separate patch)
>>
>
> OK. If you don't mind, I'll use this opportunity to also namespace
> it to ovl_cattr.

Good idea.

>
>>> +{
>>> +       struct inode *wdir = workdir->d_inode;
>>> +       struct dentry *temp;
>>> +       int err;
>>> +
>>> +       temp = ovl_lookup_temp(workdir);
>>> +       if (IS_ERR(temp))
>>> +               return temp;
>>
>> What's wrong with viro's version of  dentry in ovl_create_real()?
>> Turns this whole function into:
>>
>>         return ovl_create_real(d_inode(workdir),
>> ovl_lookup_temp(workdir), attr, hardlink, true);
>
> Nothing. A matter of taste. If you like Al's version better,
> I'll add it back in the next patch.

Al's version is the functional style yours is the procedural.  I think
in this particular case the functional version looks nicer.

Thanks,
Miklos

[PATCH v3 4/4] ovl: make ovl_create_real() cope with vfs_mkdir() safely

[Amir Goldstein]

From: Al Viro <viro@zeniv.linux.org.uk>

vfs_mkdir() may succeed and leave the dentry passed to it unhashed and
negative.  ovl_create_real() is the last caller breaking when that
happens.

Pass newdentry to ovl_create_real() by ref, so in the case above, if
lookup finds a good dentry, newdentry will be replaced by the positive
hashed one.

[amir: split re-factoring to prep patch
       pass dentry by ref
       factor out helper ovl_create_real_dir()
       add pr_warn() if mkdir succeeds and lookup fails]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 42 +++++++++++++++++++++++++++++++++++++-----
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/super.c     |  2 +-
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 45f5f9232e71..a90328c41307 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -114,9 +114,41 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 	goto out;
 }
 
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
+static int ovl_create_real_dir(struct inode *dir, struct dentry **pnewdentry,
+			       umode_t mode)
+{
+	struct dentry *newdentry = *pnewdentry;
+	int err;
+
+	err = ovl_do_mkdir(dir, newdentry, mode, true);
+	/*
+	 * vfs_mkdir() may succeed and leave the dentry passed
+	 * to it unhashed and negative. If that happens, try to
+	 * lookup a new hashed and positive dentry.
+	 */
+	if (!err && unlikely(d_unhashed(newdentry))) {
+		struct dentry *d;
+
+		d = lookup_one_len(newdentry->d_name.name,
+				   newdentry->d_parent,
+				   newdentry->d_name.len);
+		if (IS_ERR(d)) {
+			err = PTR_ERR(d);
+			pr_warn("overlayfs: failed lookup after mkdir (%pd2, err=%i).\n",
+				newdentry, err);
+			return err;
+		}
+		dput(newdentry);
+		*pnewdentry = d;
+	}
+
+	return err;
+}
+
+int ovl_create_real(struct inode *dir, struct dentry **pnewdentry,
 		    struct cattr *attr, struct dentry *hardlink, bool debug)
 {
+	struct dentry *newdentry = *pnewdentry;
 	int err;
 
 	if (newdentry->d_inode)
@@ -131,7 +163,7 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 			break;
 
 		case S_IFDIR:
-			err = ovl_do_mkdir(dir, newdentry, attr->mode, debug);
+			err = ovl_create_real_dir(dir, pnewdentry, attr->mode);
 			break;
 
 		case S_IFCHR:
@@ -150,7 +182,7 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 			err = -EPERM;
 		}
 	}
-	if (!err && WARN_ON(!newdentry->d_inode)) {
+	if (!err && WARN_ON(!d_inode(*pnewdentry))) {
 		/*
 		 * Not quite sure if non-instantiated dentry is legal or not.
 		 * VFS doesn't seem to care so check and warn here.
@@ -171,7 +203,7 @@ struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
 	if (IS_ERR(temp))
 		return temp;
 
-	err = ovl_create_real(wdir, temp, attr, hardlink, true);
+	err = ovl_create_real(wdir, &temp, attr, hardlink, true);
 	if (err) {
 		dput(temp);
 		return ERR_PTR(err);
@@ -261,7 +293,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
 		goto out_unlock;
-	err = ovl_create_real(udir, newdentry, attr, hardlink, false);
+	err = ovl_create_real(udir, &newdentry, attr, hardlink, false);
 	if (err)
 		goto out_dput;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 967183175ef5..a84599268d10 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -378,7 +378,7 @@ struct cattr {
 	umode_t mode;
 	const char *link;
 };
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
+int ovl_create_real(struct inode *dir, struct dentry **pnewdentry,
 		    struct cattr *attr,
 		    struct dentry *hardlink, bool debug);
 struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 492d534058ae..d83c0543c03c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -603,7 +603,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 			goto retry;
 		}
 
-		err = ovl_create_real(dir, work,
+		err = ovl_create_real(dir, &work,
 				      &(struct cattr){.mode = S_IFDIR | 0},
 				      NULL, true);
 		if (err)
-- 
2.7.4