[PATCH] vfs: Don't exchange filenames unconditionally.

[PATCH] vfs: Don't exchange filenames unconditionally.

[Mikhail Efremov]

Only exchange source and destination filenames
if flags contain RENAME_EXCHANGE.
In case if executable file was running and replaced by
other file /proc/PID/exe should still show correct file name,
not the old name of the file by which it was replaced.

The scenario when this bug manifests itself was like this:
* ALT Linux uses rpm and start-stop-daemon;
* during a package upgrade rpm creates a temporary file
  for an executable to rename it upon successful unpacking;
* start-stop-daemon is run subsequently and it obtains
  the (nonexistant) temporary filename via /proc/PID/exe
  thus failing to identify the running process.

Cc: Miklos Szeredi <mszeredi@suse.cz>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org

Fixes: da1ce0670c14 "vfs: add cross-rename"
Signed-off-by: Mikhail Efremov <sem@altlinux.org>
---
 fs/dcache.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 7a5b514..3218570 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2372,7 +2372,8 @@ void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 }
 EXPORT_SYMBOL(dentry_update_name_case);
 
-static void switch_names(struct dentry *dentry, struct dentry *target)
+static void switch_names(struct dentry *dentry, struct dentry *target,
+			 bool exchange)
 {
 	if (dname_external(target)) {
 		if (dname_external(dentry)) {
@@ -2404,11 +2405,21 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
 			/*
 			 * Both are internal.
 			 */
-			unsigned int i;
-			BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long)));
-			for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) {
-				swap(((long *) &dentry->d_iname)[i],
-				     ((long *) &target->d_iname)[i]);
+			if (exchange) {
+				unsigned int i;
+
+				BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN,
+							 sizeof(long)));
+				for (i = 0; i < DNAME_INLINE_LEN / sizeof(long);
+									i++) {
+					swap(((long *) &dentry->d_iname)[i],
+					     ((long *) &target->d_iname)[i]);
+				}
+			} else {
+				memcpy(dentry->d_iname, target->d_name.name,
+						target->d_name.len + 1);
+				dentry->d_name.len = target->d_name.len;
+				return;
 			}
 		}
 	}
@@ -2510,7 +2521,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
 	list_del(&target->d_u.d_child);
 
 	/* Switch the names.. */
-	switch_names(dentry, target);
+	switch_names(dentry, target, exchange);
 	swap(dentry->d_name.hash, target->d_name.hash);
 
 	/* ... and switch the parents */
@@ -2649,7 +2660,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
 
 	dparent = dentry->d_parent;
 
-	switch_names(dentry, anon);
+	switch_names(dentry, anon, false);
 	swap(dentry->d_name.hash, anon->d_name.hash);
 
 	dentry->d_parent = dentry;
-- 
1.8.5.5

Re: [PATCH] vfs: Don't exchange filenames unconditionally.

[Mateusz Guzik]

On Thu, Sep 18, 2014 at 02:55:00PM +0400, Mikhail Efremov wrote:
> Only exchange source and destination filenames
> if flags contain RENAME_EXCHANGE.

But names are still exchanged without RENAME_EXCHANGE, provided they are
long enough.

da1ce0670c14 introduced this behaviour also for short names and this is
what you are reverting here.

That said, I think the patch is fine as an interim change, but should
note facts mentioned above.

For more info please see https://lkml.org/lkml/2014/9/7/6

I'll be posting a RFC patch for actual problem next week.

> In case if executable file was running and replaced by
> other file /proc/PID/exe should still show correct file name,
> not the old name of the file by which it was replaced.
> 




> The scenario when this bug manifests itself was like this:
> * ALT Linux uses rpm and start-stop-daemon;
> * during a package upgrade rpm creates a temporary file
>   for an executable to rename it upon successful unpacking;
> * start-stop-daemon is run subsequently and it obtains
>   the (nonexistant) temporary filename via /proc/PID/exe
>   thus failing to identify the running process.
> 
> Cc: Miklos Szeredi <mszeredi@suse.cz>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> 
> Fixes: da1ce0670c14 "vfs: add cross-rename"
> Signed-off-by: Mikhail Efremov <sem@altlinux.org>
> ---
>  fs/dcache.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 7a5b514..3218570 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2372,7 +2372,8 @@ void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
>  }
>  EXPORT_SYMBOL(dentry_update_name_case);
>  
> -static void switch_names(struct dentry *dentry, struct dentry *target)
> +static void switch_names(struct dentry *dentry, struct dentry *target,
> +			 bool exchange)
>  {
>  	if (dname_external(target)) {
>  		if (dname_external(dentry)) {
> @@ -2404,11 +2405,21 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
>  			/*
>  			 * Both are internal.
>  			 */
> -			unsigned int i;
> -			BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long)));
> -			for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) {
> -				swap(((long *) &dentry->d_iname)[i],
> -				     ((long *) &target->d_iname)[i]);
> +			if (exchange) {
> +				unsigned int i;
> +
> +				BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN,
> +							 sizeof(long)));
> +				for (i = 0; i < DNAME_INLINE_LEN / sizeof(long);
> +									i++) {
> +					swap(((long *) &dentry->d_iname)[i],
> +					     ((long *) &target->d_iname)[i]);
> +				}
> +			} else {
> +				memcpy(dentry->d_iname, target->d_name.name,
> +						target->d_name.len + 1);
> +				dentry->d_name.len = target->d_name.len;
> +				return;
>  			}
>  		}
>  	}
> @@ -2510,7 +2521,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
>  	list_del(&target->d_u.d_child);
>  
>  	/* Switch the names.. */
> -	switch_names(dentry, target);
> +	switch_names(dentry, target, exchange);
>  	swap(dentry->d_name.hash, target->d_name.hash);
>  
>  	/* ... and switch the parents */
> @@ -2649,7 +2660,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
>  
>  	dparent = dentry->d_parent;
>  
> -	switch_names(dentry, anon);
> +	switch_names(dentry, anon, false);
>  	swap(dentry->d_name.hash, anon->d_name.hash);
>  
>  	dentry->d_parent = dentry;
> -- 
> 1.8.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Mateusz Guzik

Re: [PATCH] vfs: Don't exchange filenames unconditionally.

[Mikhail Efremov]

On Thu, 18 Sep 2014 13:22:07 +0200 Mateusz Guzik wrote:
> On Thu, Sep 18, 2014 at 02:55:00PM +0400, Mikhail Efremov wrote:
> > Only exchange source and destination filenames
> > if flags contain RENAME_EXCHANGE.
> 
> But names are still exchanged without RENAME_EXCHANGE, provided they are
> long enough.
> 
> da1ce0670c14 introduced this behaviour also for short names and this is
> what you are reverting here.
> 
> That said, I think the patch is fine as an interim change, but should
> note facts mentioned above.
> 
> For more info please see https://lkml.org/lkml/2014/9/7/6

Thanks for pointing out that thread to me, I didn't read it before
sending the patch.

> I'll be posting a RFC patch for actual problem next week.

I suppose then that no sense to resend this patch right now, I'll just
wait the proper solution from you.
Yet this patch can be used as a temporary workaround (as you mentioned
above) if the proper solution takes longer than expected to implement.

> > In case if executable file was running and replaced by
> > other file /proc/PID/exe should still show correct file name,
> > not the old name of the file by which it was replaced.
> 
> 
> 
> 
> > The scenario when this bug manifests itself was like this:
> > * ALT Linux uses rpm and start-stop-daemon;
> > * during a package upgrade rpm creates a temporary file
> >   for an executable to rename it upon successful unpacking;
> > * start-stop-daemon is run subsequently and it obtains
> >   the (nonexistant) temporary filename via /proc/PID/exe
> >   thus failing to identify the running process.
> > 
> > Cc: Miklos Szeredi <mszeredi@suse.cz>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: linux-fsdevel@vger.kernel.org
> > 
> > Fixes: da1ce0670c14 "vfs: add cross-rename"
> > Signed-off-by: Mikhail Efremov <sem@altlinux.org>
> > ---
> >  fs/dcache.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 7a5b514..3218570 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2372,7 +2372,8 @@ void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
> >  }
> >  EXPORT_SYMBOL(dentry_update_name_case);
> >  
> > -static void switch_names(struct dentry *dentry, struct dentry *target)
> > +static void switch_names(struct dentry *dentry, struct dentry *target,
> > +			 bool exchange)
> >  {
> >  	if (dname_external(target)) {
> >  		if (dname_external(dentry)) {
> > @@ -2404,11 +2405,21 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
> >  			/*
> >  			 * Both are internal.
> >  			 */
> > -			unsigned int i;
> > -			BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long)));
> > -			for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) {
> > -				swap(((long *) &dentry->d_iname)[i],
> > -				     ((long *) &target->d_iname)[i]);
> > +			if (exchange) {
> > +				unsigned int i;
> > +
> > +				BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN,
> > +							 sizeof(long)));
> > +				for (i = 0; i < DNAME_INLINE_LEN / sizeof(long);
> > +									i++) {
> > +					swap(((long *) &dentry->d_iname)[i],
> > +					     ((long *) &target->d_iname)[i]);
> > +				}
> > +			} else {
> > +				memcpy(dentry->d_iname, target->d_name.name,
> > +						target->d_name.len + 1);
> > +				dentry->d_name.len = target->d_name.len;
> > +				return;
> >  			}
> >  		}
> >  	}
> > @@ -2510,7 +2521,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
> >  	list_del(&target->d_u.d_child);
> >  
> >  	/* Switch the names.. */
> > -	switch_names(dentry, target);
> > +	switch_names(dentry, target, exchange);
> >  	swap(dentry->d_name.hash, target->d_name.hash);
> >  
> >  	/* ... and switch the parents */
> > @@ -2649,7 +2660,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
> >  
> >  	dparent = dentry->d_parent;
> >  
> > -	switch_names(dentry, anon);
> > +	switch_names(dentry, anon, false);
> >  	swap(dentry->d_name.hash, anon->d_name.hash);
> >  
> >  	dentry->d_parent = dentry;
> > -- 
> > 1.8.5.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
WBR, Mikhail Efremov

Re: [PATCH] vfs: Don't exchange filenames unconditionally.

[Miklos Szeredi]

On Fri, Sep 19, 2014 at 4:22 PM, Mikhail Efremov <sem@altlinux.ru> wrote:
> On Thu, 18 Sep 2014 13:22:07 +0200 Mateusz Guzik wrote:
>> On Thu, Sep 18, 2014 at 02:55:00PM +0400, Mikhail Efremov wrote:
>> > Only exchange source and destination filenames
>> > if flags contain RENAME_EXCHANGE.

Acked-by: Miklos Szeredi <mszeredi@suse.cz>


>>
>> But names are still exchanged without RENAME_EXCHANGE, provided they are
>> long enough.
>>
>> da1ce0670c14 introduced this behaviour also for short names and this is
>> what you are reverting here.
>>
>> That said, I think the patch is fine as an interim change, but should
>> note facts mentioned above.
>>
>> For more info please see https://lkml.org/lkml/2014/9/7/6
>
> Thanks for pointing out that thread to me, I didn't read it before
> sending the patch.
>
>> I'll be posting a RFC patch for actual problem next week.
>
> I suppose then that no sense to resend this patch right now, I'll just
> wait the proper solution from you.
> Yet this patch can be used as a temporary workaround (as you mentioned
> above) if the proper solution takes longer than expected to implement.

Yes, feel free to resend it to Linus, before 3.17 proper is out.

Also please consider adding:

Cc: stable@vger.kernel.org

Thanks,
Miklos