* [PATCH] vfs: Don't exchange filenames unconditionally.
@ 2014-09-18 10:55 Mikhail Efremov
2014-09-18 11:22 ` Mateusz Guzik
0 siblings, 1 reply; 4+ messages in thread
From: Mikhail Efremov @ 2014-09-18 10:55 UTC (permalink / raw)
To: linux-kernel
Cc: Mikhail Efremov, Miklos Szeredi, Alexander Viro, linux-fsdevel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] vfs: Don't exchange filenames unconditionally.
2014-09-18 10:55 [PATCH] vfs: Don't exchange filenames unconditionally Mikhail Efremov
@ 2014-09-18 11:22 ` Mateusz Guzik
2014-09-19 14:22 ` Mikhail Efremov
0 siblings, 1 reply; 4+ messages in thread
From: Mateusz Guzik @ 2014-09-18 11:22 UTC (permalink / raw)
To: Mikhail Efremov
Cc: linux-kernel, Miklos Szeredi, Alexander Viro, linux-fsdevel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] vfs: Don't exchange filenames unconditionally.
2014-09-18 11:22 ` Mateusz Guzik
@ 2014-09-19 14:22 ` Mikhail Efremov
2014-09-23 16:19 ` Miklos Szeredi
0 siblings, 1 reply; 4+ messages in thread
From: Mikhail Efremov @ 2014-09-19 14:22 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Mikhail Efremov, linux-kernel, Miklos Szeredi, Alexander Viro,
linux-fsdevel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] vfs: Don't exchange filenames unconditionally.
2014-09-19 14:22 ` Mikhail Efremov
@ 2014-09-23 16:19 ` Miklos Szeredi
0 siblings, 0 replies; 4+ messages in thread
From: Miklos Szeredi @ 2014-09-23 16:19 UTC (permalink / raw)
To: Mikhail Efremov
Cc: Mateusz Guzik, Mikhail Efremov, LKML, Miklos Szeredi,
Alexander Viro, linux-fsdevel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-23 16:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-18 10:55 [PATCH] vfs: Don't exchange filenames unconditionally Mikhail Efremov
2014-09-18 11:22 ` Mateusz Guzik
2014-09-19 14:22 ` Mikhail Efremov
2014-09-23 16:19 ` Miklos Szeredi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).