[Devel,rh7] fs: avoid holding extra reference on original path

Submitted by Maxim Patlasov on Oct. 1, 2016, 1:04 a.m.

Details

Message ID 147528383809.17748.1988120255891477887.stgit@maxim-thinkpad
State New
Series "fs: avoid holding extra reference on original path"
Headers show

Commit Message

Maxim Patlasov Oct. 1, 2016, 1:04 a.m.
When vfs_open() opens ordinary file (not overlayfs one), do_dentry_open() will
call path_get(&f->f_path) anyway, so it doesn't make sense to acquire extra
references by another path_get().

The above is enough to satisfy LTP syscalls/fcntl/fcntl24 when called on top
of ordinary fs (not overlayfs), but for overlayfs it's also necessary to ensure
that fs/locks.c::generic_add_lease() passes original (overlayfs) dentry to
check_conflicting_open(). This goes in accordance with mainstream:

> commit 6343a2120862f7023006c8091ad95c1f16a32077
> Author: Miklos Szeredi <mszeredi@redhat.com>
> Date:   Fri Jul 1 14:56:07 2016 +0200
>
>     locks: use file_inode()
>
>     (Another one for the f_path debacle.)
>
>     ltp fcntl33 testcase caused an Oops in selinux_file_send_sigiotask.
>
>     The reason is that generic_add_lease() used filp->f_path.dentry->inode
>     while all the others use file_inode().  This makes a difference for files
>     opened on overlayfs since the former will point to the overlay inode the
>     latter to the underlying inode.
>
>     So generic_add_lease() added the lease to the overlay inode and
>     generic_delete_lease() removed it from the underlying inode.  When the file
>     was released the lease remained on the overlay inode's lock list, resulting
>     in use after free.
>
>     Reported-by: Eryu Guan <eguan@redhat.com>
>     Fixes: 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and f_inode to the underlay")
>     Cc: <stable@vger.kernel.org>
>     Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>     Reviewed-by: Jeff Layton <jlayton@redhat.com>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 7c5f91b..ee1b15f 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1628,7 +1628,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>  {
>         struct file_lock *fl, *my_fl = NULL, *lease;
>         struct dentry *dentry = filp->f_path.dentry;
> -       struct inode *inode = dentry->d_inode;
> +       struct inode *inode = file_inode(filp);
>         struct file_lock_context *ctx;
>         bool is_deleg = (*flp)->fl_flags & FL_DELEG;
>         int error;

https://jira.sw.ru/browse/PSBM-52817

Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
---
 fs/locks.c |    5 +++--
 fs/open.c  |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/locks.c b/fs/locks.c
index a5ab0c0..f6c89d7 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1560,8 +1560,9 @@  static int
 generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
 {
 	struct file_lock *fl, **before, **my_before = NULL, *lease;
-	struct dentry *dentry = filp->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
+	struct dentry *dentry = filp->f_original_path.mnt ?
+		filp->f_original_path.dentry: filp->f_path.dentry;
+	struct inode *inode = filp->f_path.dentry->d_inode;
 	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
 	int error;
 
diff --git a/fs/open.c b/fs/open.c
index 25dbc85..84eb289 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -896,7 +896,7 @@  int vfs_open(const struct path *path, struct file *filp,
 	int do_cleanup = 0;
 	int ret;
 
-	if (!filp->f_original_path.mnt) {
+	if (!filp->f_original_path.mnt && dentry_open) {
 		filp->f_original_path = *path;
 		path_get(&filp->f_original_path);
 		do_cleanup = 1;

Comments

Kirill Tkhai Oct. 3, 2016, 9:24 a.m.
On 01.10.2016 04:04, Maxim Patlasov wrote:
> When vfs_open() opens ordinary file (not overlayfs one), do_dentry_open() will
> call path_get(&f->f_path) anyway, so it doesn't make sense to acquire extra
> references by another path_get().
> 
> The above is enough to satisfy LTP syscalls/fcntl/fcntl24 when called on top
> of ordinary fs (not overlayfs), but for overlayfs it's also necessary to ensure
> that fs/locks.c::generic_add_lease() passes original (overlayfs) dentry to
> check_conflicting_open(). This goes in accordance with mainstream:
> 
>> commit 6343a2120862f7023006c8091ad95c1f16a32077
>> Author: Miklos Szeredi <mszeredi@redhat.com>
>> Date:   Fri Jul 1 14:56:07 2016 +0200
>>
>>     locks: use file_inode()
>>
>>     (Another one for the f_path debacle.)
>>
>>     ltp fcntl33 testcase caused an Oops in selinux_file_send_sigiotask.
>>
>>     The reason is that generic_add_lease() used filp->f_path.dentry->inode
>>     while all the others use file_inode().  This makes a difference for files
>>     opened on overlayfs since the former will point to the overlay inode the
>>     latter to the underlying inode.
>>
>>     So generic_add_lease() added the lease to the overlay inode and
>>     generic_delete_lease() removed it from the underlying inode.  When the file
>>     was released the lease remained on the overlay inode's lock list, resulting
>>     in use after free.
>>
>>     Reported-by: Eryu Guan <eguan@redhat.com>
>>     Fixes: 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and f_inode to the underlay")
>>     Cc: <stable@vger.kernel.org>
>>     Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>>     Reviewed-by: Jeff Layton <jlayton@redhat.com>
>>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index 7c5f91b..ee1b15f 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -1628,7 +1628,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>>  {
>>         struct file_lock *fl, *my_fl = NULL, *lease;
>>         struct dentry *dentry = filp->f_path.dentry;
>> -       struct inode *inode = dentry->d_inode;
>> +       struct inode *inode = file_inode(filp);
>>         struct file_lock_context *ctx;
>>         bool is_deleg = (*flp)->fl_flags & FL_DELEG;
>>         int error;
> 
> https://jira.sw.ru/browse/PSBM-52817
> 
> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  fs/locks.c |    5 +++--
>  fs/open.c  |    2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index a5ab0c0..f6c89d7 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1560,8 +1560,9 @@ static int
>  generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
>  {
>  	struct file_lock *fl, **before, **my_before = NULL, *lease;
> -	struct dentry *dentry = filp->f_path.dentry;
> -	struct inode *inode = dentry->d_inode;
> +	struct dentry *dentry = filp->f_original_path.mnt ?
> +		filp->f_original_path.dentry: filp->f_path.dentry;
> +	struct inode *inode = filp->f_path.dentry->d_inode;
>  	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
>  	int error;
>  
> diff --git a/fs/open.c b/fs/open.c
> index 25dbc85..84eb289 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -896,7 +896,7 @@ int vfs_open(const struct path *path, struct file *filp,
>  	int do_cleanup = 0;
>  	int ret;
>  
> -	if (!filp->f_original_path.mnt) {
> +	if (!filp->f_original_path.mnt && dentry_open) {
>  		filp->f_original_path = *path;
>  		path_get(&filp->f_original_path);
>  		do_cleanup = 1;
>