* [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
@ 2017-03-30 8:56 Richard Weinberger
2017-03-30 8:59 ` Amir Goldstein
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Richard Weinberger @ 2017-03-30 8:56 UTC (permalink / raw)
To: linux-mtd
Cc: linux-kernel, adrian.hunter, dedekind1, richard, stable,
Ralph Sennhauser, Amir Goldstein
It is perfectly fine to link a tmpfile back using linkat().
Since tmpfiles are created with a link count of 0 they appear
on the orphan list, upon re-linking the inode has to be removed
from the orphan list again.
Cc: <stable@vger.kernel.org>
Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Cc: Amir Goldstein <amir73il@gmail.com
Reported-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Tested-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Reported-by: Amir Goldstein <amir73il@gmail.com
Fixes: 474b93704f321 ("ubifs: Implement O_TMPFILE")
Signed-off-by: Richard Weinberger <richard@nod.at>
---
fs/ubifs/dir.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 0858213a4e63..0139155045fe 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
goto out_fname;
lock_2_inodes(dir, inode);
+
+ /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
+ if (inode->i_nlink == 0)
+ ubifs_delete_orphan(c, inode->i_ino);
+
inc_nlink(inode);
ihold(inode);
inode->i_ctime = ubifs_current_time(inode);
--
2.7.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 8:56 [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger
@ 2017-03-30 8:59 ` Amir Goldstein
2017-03-30 9:03 ` Richard Weinberger
2017-03-30 9:10 ` Ralph Sennhauser
2017-03-30 9:32 ` Adrian Hunter
2017-03-30 10:34 ` Hyunchul Lee
2 siblings, 2 replies; 22+ messages in thread
From: Amir Goldstein @ 2017-03-30 8:59 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-mtd, linux-kernel, Adrian Hunter, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
On Thu, Mar 30, 2017 at 11:56 AM, Richard Weinberger <richard@nod.at> wrote:
> It is perfectly fine to link a tmpfile back using linkat().
> Since tmpfiles are created with a link count of 0 they appear
> on the orphan list, upon re-linking the inode has to be removed
> from the orphan list again.
>
Looks good.
> Cc: <stable@vger.kernel.org>
> Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Cc: Amir Goldstein <amir73il@gmail.com
typo: missing closing >
> Reported-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Reported-by: Amir Goldstein <amir73il@gmail.com
and here too
> Fixes: 474b93704f321 ("ubifs: Implement O_TMPFILE")
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> fs/ubifs/dir.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 0858213a4e63..0139155045fe 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
> goto out_fname;
>
> lock_2_inodes(dir, inode);
> +
> + /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
> + if (inode->i_nlink == 0)
> + ubifs_delete_orphan(c, inode->i_ino);
> +
> inc_nlink(inode);
> ihold(inode);
> inode->i_ctime = ubifs_current_time(inode);
> --
> 2.7.3
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 8:59 ` Amir Goldstein
@ 2017-03-30 9:03 ` Richard Weinberger
2017-03-30 9:07 ` Amir Goldstein
2017-03-30 9:10 ` Ralph Sennhauser
1 sibling, 1 reply; 22+ messages in thread
From: Richard Weinberger @ 2017-03-30 9:03 UTC (permalink / raw)
To: Amir Goldstein
Cc: linux-mtd, linux-kernel, Adrian Hunter, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
Amir,
Am 30.03.2017 um 10:59 schrieb Amir Goldstein:
> On Thu, Mar 30, 2017 at 11:56 AM, Richard Weinberger <richard@nod.at> wrote:
>> It is perfectly fine to link a tmpfile back using linkat().
>> Since tmpfiles are created with a link count of 0 they appear
>> on the orphan list, upon re-linking the inode has to be removed
>> from the orphan list again.
>>
>
> Looks good.
>
>> Cc: <stable@vger.kernel.org>
>> Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
>> Cc: Amir Goldstein <amir73il@gmail.com
>
> typo: missing closing >
Whoops, copied one byte too few from Thunderbird. :D
Will fix before pushing.
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 9:03 ` Richard Weinberger
@ 2017-03-30 9:07 ` Amir Goldstein
2017-03-30 9:09 ` Richard Weinberger
0 siblings, 1 reply; 22+ messages in thread
From: Amir Goldstein @ 2017-03-30 9:07 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-mtd, linux-kernel, Adrian Hunter, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
On Thu, Mar 30, 2017 at 12:03 PM, Richard Weinberger <richard@nod.at> wrote:
> Amir,
>
> Am 30.03.2017 um 10:59 schrieb Amir Goldstein:
>> On Thu, Mar 30, 2017 at 11:56 AM, Richard Weinberger <richard@nod.at> wrote:
>>> It is perfectly fine to link a tmpfile back using linkat().
>>> Since tmpfiles are created with a link count of 0 they appear
>>> on the orphan list, upon re-linking the inode has to be removed
>>> from the orphan list again.
>>>
>>
>> Looks good.
>>
>>> Cc: <stable@vger.kernel.org>
>>> Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
>>> Cc: Amir Goldstein <amir73il@gmail.com
>>
>> typo: missing closing >
>
> Whoops, copied one byte too few from Thunderbird. :D
> Will fix before pushing.
>
It's worth mentioning the bug that Ralph reported.
The fact that overlayfs mount in v4.11-rc causes corruption of ubifs is much
more severe than what the current commit message implies (fix of a corner case).
I would also add # v4.9 hint to stable tag, just to be nice ;-)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 9:07 ` Amir Goldstein
@ 2017-03-30 9:09 ` Richard Weinberger
0 siblings, 0 replies; 22+ messages in thread
From: Richard Weinberger @ 2017-03-30 9:09 UTC (permalink / raw)
To: Amir Goldstein
Cc: linux-mtd, linux-kernel, Adrian Hunter, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
Amir,
Am 30.03.2017 um 11:07 schrieb Amir Goldstein:
> On Thu, Mar 30, 2017 at 12:03 PM, Richard Weinberger <richard@nod.at> wrote:
>> Amir,
>>
>> Am 30.03.2017 um 10:59 schrieb Amir Goldstein:
>>> On Thu, Mar 30, 2017 at 11:56 AM, Richard Weinberger <richard@nod.at> wrote:
>>>> It is perfectly fine to link a tmpfile back using linkat().
>>>> Since tmpfiles are created with a link count of 0 they appear
>>>> on the orphan list, upon re-linking the inode has to be removed
>>>> from the orphan list again.
>>>>
>>>
>>> Looks good.
>>>
>>>> Cc: <stable@vger.kernel.org>
>>>> Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
>>>> Cc: Amir Goldstein <amir73il@gmail.com
>>>
>>> typo: missing closing >
>>
>> Whoops, copied one byte too few from Thunderbird. :D
>> Will fix before pushing.
>>
>
>
> It's worth mentioning the bug that Ralph reported.
> The fact that overlayfs mount in v4.11-rc causes corruption of ubifs is much
> more severe than what the current commit message implies (fix of a corner case).
Okay, I'll add more drama to the commit message.
Usually I assume that stable patches for filesystems are considered as something
serious.
> I would also add # v4.9 hint to stable tag, just to be nice ;-)
Isn't this why we have the Fixes tag?
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 8:59 ` Amir Goldstein
2017-03-30 9:03 ` Richard Weinberger
@ 2017-03-30 9:10 ` Ralph Sennhauser
1 sibling, 0 replies; 22+ messages in thread
From: Ralph Sennhauser @ 2017-03-30 9:10 UTC (permalink / raw)
To: Amir Goldstein
Cc: Richard Weinberger, linux-mtd, linux-kernel, Adrian Hunter,
Artem Bityutskiy, stable [v4.9]
On Thu, 30 Mar 2017 11:59:17 +0300
Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Mar 30, 2017 at 11:56 AM, Richard Weinberger <richard@nod.at>
> wrote:
> > It is perfectly fine to link a tmpfile back using linkat().
> > Since tmpfiles are created with a link count of 0 they appear
> > on the orphan list, upon re-linking the inode has to be removed
> > from the orphan list again.
> >
>
> Looks good.
Nothing to add.
Thanks to both of you.
Ralph
>
> > Cc: <stable@vger.kernel.org>
> > Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> > Cc: Amir Goldstein <amir73il@gmail.com
>
> typo: missing closing >
>
> > Reported-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> > Tested-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> > Reported-by: Amir Goldstein <amir73il@gmail.com
>
> and here too
>
> > Fixes: 474b93704f321 ("ubifs: Implement O_TMPFILE")
> > Signed-off-by: Richard Weinberger <richard@nod.at>
> > ---
> > fs/ubifs/dir.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> > index 0858213a4e63..0139155045fe 100644
> > --- a/fs/ubifs/dir.c
> > +++ b/fs/ubifs/dir.c
> > @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry
> > *old_dentry, struct inode *dir, goto out_fname;
> >
> > lock_2_inodes(dir, inode);
> > +
> > + /* Handle O_TMPFILE corner case, it is allowed to link a
> > O_TMPFILE. */
> > + if (inode->i_nlink == 0)
> > + ubifs_delete_orphan(c, inode->i_ino);
> > +
> > inc_nlink(inode);
> > ihold(inode);
> > inode->i_ctime = ubifs_current_time(inode);
> > --
> > 2.7.3
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 8:56 [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger
2017-03-30 8:59 ` Amir Goldstein
@ 2017-03-30 9:32 ` Adrian Hunter
2017-03-30 9:49 ` Richard Weinberger
2017-03-30 10:34 ` Hyunchul Lee
2 siblings, 1 reply; 22+ messages in thread
From: Adrian Hunter @ 2017-03-30 9:32 UTC (permalink / raw)
To: Richard Weinberger, linux-mtd
Cc: linux-kernel, dedekind1, stable, Ralph Sennhauser, Amir Goldstein
On 30/03/17 11:56, Richard Weinberger wrote:
> It is perfectly fine to link a tmpfile back using linkat().
> Since tmpfiles are created with a link count of 0 they appear
> on the orphan list, upon re-linking the inode has to be removed
> from the orphan list again.
>
> Cc: <stable@vger.kernel.org>
> Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Cc: Amir Goldstein <amir73il@gmail.com
> Reported-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Reported-by: Amir Goldstein <amir73il@gmail.com
> Fixes: 474b93704f321 ("ubifs: Implement O_TMPFILE")
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> fs/ubifs/dir.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 0858213a4e63..0139155045fe 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
> goto out_fname;
>
> lock_2_inodes(dir, inode);
> +
> + /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
> + if (inode->i_nlink == 0)
> + ubifs_delete_orphan(c, inode->i_ino);
Isn't there also a deletion inode in the journal? If the recovery sees that
won't it delete the file data?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 9:32 ` Adrian Hunter
@ 2017-03-30 9:49 ` Richard Weinberger
2017-03-30 10:23 ` Richard Weinberger
0 siblings, 1 reply; 22+ messages in thread
From: Richard Weinberger @ 2017-03-30 9:49 UTC (permalink / raw)
To: Adrian Hunter, linux-mtd
Cc: linux-kernel, dedekind1, stable, Ralph Sennhauser, Amir Goldstein
Am 30.03.2017 um 11:32 schrieb Adrian Hunter:
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index 0858213a4e63..0139155045fe 100644
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
>> goto out_fname;
>>
>> lock_2_inodes(dir, inode);
>> +
>> + /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
>> + if (inode->i_nlink == 0)
>> + ubifs_delete_orphan(c, inode->i_ino);
>
> Isn't there also a deletion inode in the journal? If the recovery sees that
> won't it delete the file data?
Yes, but ubifs_link() adds a new journal entry which revives the inode.
This should cancel out the deletion, right?
You know the UBIFS journal better than I do. :-)
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 9:49 ` Richard Weinberger
@ 2017-03-30 10:23 ` Richard Weinberger
2017-03-30 10:35 ` Amir Goldstein
2017-03-30 11:57 ` Adrian Hunter
0 siblings, 2 replies; 22+ messages in thread
From: Richard Weinberger @ 2017-03-30 10:23 UTC (permalink / raw)
To: Adrian Hunter, linux-mtd
Cc: linux-kernel, dedekind1, stable, Ralph Sennhauser, Amir Goldstein
Am 30.03.2017 um 11:49 schrieb Richard Weinberger:
> Am 30.03.2017 um 11:32 schrieb Adrian Hunter:
>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>> index 0858213a4e63..0139155045fe 100644
>>> --- a/fs/ubifs/dir.c
>>> +++ b/fs/ubifs/dir.c
>>> @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
>>> goto out_fname;
>>>
>>> lock_2_inodes(dir, inode);
>>> +
>>> + /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
>>> + if (inode->i_nlink == 0)
>>> + ubifs_delete_orphan(c, inode->i_ino);
>>
>> Isn't there also a deletion inode in the journal? If the recovery sees that
>> won't it delete the file data?
>
> Yes, but ubifs_link() adds a new journal entry which revives the inode.
> This should cancel out the deletion, right?
> You know the UBIFS journal better than I do. :-)
Reading deeper into the proved that I was wrong.
AFAIKT UBIFS' journal has currently no way to revive a deleted inode.
So, we have to think about a new solution.
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 8:56 [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger
2017-03-30 8:59 ` Amir Goldstein
2017-03-30 9:32 ` Adrian Hunter
@ 2017-03-30 10:34 ` Hyunchul Lee
2 siblings, 0 replies; 22+ messages in thread
From: Hyunchul Lee @ 2017-03-30 10:34 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-mtd, dedekind1, Amir Goldstein, linux-kernel, stable,
adrian.hunter, Ralph Sennhauser, kernel-team
[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]
Richard,
On Thu, Mar 30, 2017 at 10:56:21AM +0200, Richard Weinberger wrote:
> It is perfectly fine to link a tmpfile back using linkat().
> Since tmpfiles are created with a link count of 0 they appear
> on the orphan list, upon re-linking the inode has to be removed
> from the orphan list again.
>
> Cc: <stable@vger.kernel.org>
> Cc: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Cc: Amir Goldstein <amir73il@gmail.com
> Reported-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Reported-by: Amir Goldstein <amir73il@gmail.com
> Fixes: 474b93704f321 ("ubifs: Implement O_TMPFILE")
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> fs/ubifs/dir.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 0858213a4e63..0139155045fe 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
> goto out_fname;
>
> lock_2_inodes(dir, inode);
> +
> + /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
> + if (inode->i_nlink == 0)
> + ubifs_delete_orphan(c, inode->i_ino);
> +
I guess that ubifs_delete_orphan should be called if ubifs_jnl_update
succeeds.
> inc_nlink(inode);
> ihold(inode);
> inode->i_ctime = ubifs_current_time(inode);
> --
> 2.7.3
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
[-- Attachment #2: fix-unlink --]
[-- Type: text/plain, Size: 422 bytes --]
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 528369f..a2e4a4b 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -757,6 +757,8 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0);
if (err)
goto out_cancel;
+ if (inode->i_nlink == 1)
+ ubifs_delete_orphan(c, inode->i_ino);
unlock_2_inodes(dir, inode);
ubifs_release_budget(c, &req);
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 10:23 ` Richard Weinberger
@ 2017-03-30 10:35 ` Amir Goldstein
2017-03-30 10:51 ` Richard Weinberger
2017-03-30 11:57 ` Adrian Hunter
1 sibling, 1 reply; 22+ messages in thread
From: Amir Goldstein @ 2017-03-30 10:35 UTC (permalink / raw)
To: Richard Weinberger
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
On Thu, Mar 30, 2017 at 1:23 PM, Richard Weinberger <richard@nod.at> wrote:
> Am 30.03.2017 um 11:49 schrieb Richard Weinberger:
>> Am 30.03.2017 um 11:32 schrieb Adrian Hunter:
>>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>>> index 0858213a4e63..0139155045fe 100644
>>>> --- a/fs/ubifs/dir.c
>>>> +++ b/fs/ubifs/dir.c
>>>> @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
>>>> goto out_fname;
>>>>
>>>> lock_2_inodes(dir, inode);
>>>> +
>>>> + /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
>>>> + if (inode->i_nlink == 0)
>>>> + ubifs_delete_orphan(c, inode->i_ino);
>>>
>>> Isn't there also a deletion inode in the journal? If the recovery sees that
>>> won't it delete the file data?
>>
>> Yes, but ubifs_link() adds a new journal entry which revives the inode.
>> This should cancel out the deletion, right?
>> You know the UBIFS journal better than I do. :-)
>
> Reading deeper into the proved that I was wrong.
> AFAIKT UBIFS' journal has currently no way to revive a deleted inode.
> So, we have to think about a new solution.
>
Not that I know anything about ubifs, but why do you need the deleted
inode record in the first place for an O_TMPFILE.
vfs ensures you that you can only link back an O_TMPFILE, not a deleted
inode.
It does not appear to be the right thing to do to pass deletion=1 to
ubifs_jnl_update(), but deletion=0 doesn't look right as well..
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 10:35 ` Amir Goldstein
@ 2017-03-30 10:51 ` Richard Weinberger
0 siblings, 0 replies; 22+ messages in thread
From: Richard Weinberger @ 2017-03-30 10:51 UTC (permalink / raw)
To: Amir Goldstein
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
Amir,
Am 30.03.2017 um 12:35 schrieb Amir Goldstein:
>> Reading deeper into the proved that I was wrong.
>> AFAIKT UBIFS' journal has currently no way to revive a deleted inode.
>> So, we have to think about a new solution.
>>
>
> Not that I know anything about ubifs, but why do you need the deleted
> inode record in the first place for an O_TMPFILE.
> vfs ensures you that you can only link back an O_TMPFILE, not a deleted
> inode.
>
> It does not appear to be the right thing to do to pass deletion=1 to
> ubifs_jnl_update(), but deletion=0 doesn't look right as well..
We need to think of a new case.
I choose deletion=1 to ensure that after a power-cut written data of the
tmpfile gets removed.
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 10:23 ` Richard Weinberger
2017-03-30 10:35 ` Amir Goldstein
@ 2017-03-30 11:57 ` Adrian Hunter
2017-03-30 12:27 ` Richard Weinberger
1 sibling, 1 reply; 22+ messages in thread
From: Adrian Hunter @ 2017-03-30 11:57 UTC (permalink / raw)
To: Richard Weinberger, linux-mtd
Cc: linux-kernel, dedekind1, stable, Ralph Sennhauser, Amir Goldstein
On 30/03/17 13:23, Richard Weinberger wrote:
> Am 30.03.2017 um 11:49 schrieb Richard Weinberger:
>> Am 30.03.2017 um 11:32 schrieb Adrian Hunter:
>>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>>> index 0858213a4e63..0139155045fe 100644
>>>> --- a/fs/ubifs/dir.c
>>>> +++ b/fs/ubifs/dir.c
>>>> @@ -748,6 +748,11 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
>>>> goto out_fname;
>>>>
>>>> lock_2_inodes(dir, inode);
>>>> +
>>>> + /* Handle O_TMPFILE corner case, it is allowed to link a O_TMPFILE. */
>>>> + if (inode->i_nlink == 0)
>>>> + ubifs_delete_orphan(c, inode->i_ino);
>>>
>>> Isn't there also a deletion inode in the journal? If the recovery sees that
>>> won't it delete the file data?
>>
>> Yes, but ubifs_link() adds a new journal entry which revives the inode.
>> This should cancel out the deletion, right?
>> You know the UBIFS journal better than I do. :-)
>
> Reading deeper into the proved that I was wrong.
> AFAIKT UBIFS' journal has currently no way to revive a deleted inode.
> So, we have to think about a new solution.
Deleting the orphan looks right. Just need to understand whether the
recovery would do the right thing - actually it looks like O_TMPFILE might
be OK and in other case we might be failing to remove nodes with sequence
numbers greater than the deletion inode.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 11:57 ` Adrian Hunter
@ 2017-03-30 12:27 ` Richard Weinberger
2017-04-06 12:06 ` Amir Goldstein
0 siblings, 1 reply; 22+ messages in thread
From: Richard Weinberger @ 2017-03-30 12:27 UTC (permalink / raw)
To: Adrian Hunter, linux-mtd
Cc: linux-kernel, dedekind1, stable, Ralph Sennhauser, Amir Goldstein
Am 30.03.2017 um 13:57 schrieb Adrian Hunter:
>> Reading deeper into the proved that I was wrong.
>> AFAIKT UBIFS' journal has currently no way to revive a deleted inode.
>> So, we have to think about a new solution.
>
> Deleting the orphan looks right. Just need to understand whether the
> recovery would do the right thing - actually it looks like O_TMPFILE might
> be OK and in other case we might be failing to remove nodes with sequence
> numbers greater than the deletion inode.
Sadly it does not the right thing.
I'm currently investigating why and how to deal with it.
I also managed to trigger that case. :(
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-03-30 12:27 ` Richard Weinberger
@ 2017-04-06 12:06 ` Amir Goldstein
2017-04-06 12:09 ` Richard Weinberger
0 siblings, 1 reply; 22+ messages in thread
From: Amir Goldstein @ 2017-04-06 12:06 UTC (permalink / raw)
To: Richard Weinberger
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
On Thu, Mar 30, 2017 at 3:27 PM, Richard Weinberger <richard@nod.at> wrote:
> Am 30.03.2017 um 13:57 schrieb Adrian Hunter:
>>> Reading deeper into the proved that I was wrong.
>>> AFAIKT UBIFS' journal has currently no way to revive a deleted inode.
>>> So, we have to think about a new solution.
>>
>> Deleting the orphan looks right. Just need to understand whether the
>> recovery would do the right thing - actually it looks like O_TMPFILE might
>> be OK and in other case we might be failing to remove nodes with sequence
>> numbers greater than the deletion inode.
>
> Sadly it does not the right thing.
> I'm currently investigating why and how to deal with it.
>
> I also managed to trigger that case. :(
>
Richard,
Were you able to make any progress? still working on this?
If this is too complicated to get in for this cycle, better send a patch
to disable O_TMPFILE support for ubifs and fix the problem properly on
followup merge cycle.
Because right now ubifs O_TMPFILE support is broken and breaks overlayfs mount.
Amir.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-04-06 12:06 ` Amir Goldstein
@ 2017-04-06 12:09 ` Richard Weinberger
2017-04-06 12:26 ` Richard Weinberger
0 siblings, 1 reply; 22+ messages in thread
From: Richard Weinberger @ 2017-04-06 12:09 UTC (permalink / raw)
To: Amir Goldstein
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
Amir,
Am 06.04.2017 um 14:06 schrieb Amir Goldstein:
> On Thu, Mar 30, 2017 at 3:27 PM, Richard Weinberger <richard@nod.at> wrote:
>> Am 30.03.2017 um 13:57 schrieb Adrian Hunter:
>>>> Reading deeper into the proved that I was wrong.
>>>> AFAIKT UBIFS' journal has currently no way to revive a deleted inode.
>>>> So, we have to think about a new solution.
>>>
>>> Deleting the orphan looks right. Just need to understand whether the
>>> recovery would do the right thing - actually it looks like O_TMPFILE might
>>> be OK and in other case we might be failing to remove nodes with sequence
>>> numbers greater than the deletion inode.
>>
>> Sadly it does not the right thing.
>> I'm currently investigating why and how to deal with it.
>>
>> I also managed to trigger that case. :(
>>
>
> Richard,
>
> Were you able to make any progress? still working on this?
> If this is too complicated to get in for this cycle, better send a patch
> to disable O_TMPFILE support for ubifs and fix the problem properly on
> followup merge cycle.
> Because right now ubifs O_TMPFILE support is broken and breaks overlayfs mount.
I have a test and currently testing it. As it looks the situation is less worse
than I thought first. :-)
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-04-06 12:09 ` Richard Weinberger
@ 2017-04-06 12:26 ` Richard Weinberger
2017-04-11 10:20 ` Amir Goldstein
0 siblings, 1 reply; 22+ messages in thread
From: Richard Weinberger @ 2017-04-06 12:26 UTC (permalink / raw)
To: Amir Goldstein
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
Am 06.04.2017 um 14:09 schrieb Richard Weinberger:
>> Were you able to make any progress? still working on this?
>> If this is too complicated to get in for this cycle, better send a patch
>> to disable O_TMPFILE support for ubifs and fix the problem properly on
>> followup merge cycle.
>> Because right now ubifs O_TMPFILE support is broken and breaks overlayfs mount.
>
> I have a test and currently testing it. As it looks the situation is less worse
> than I thought first. :-)
s/test/patch :)
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-04-06 12:26 ` Richard Weinberger
@ 2017-04-11 10:20 ` Amir Goldstein
2017-04-11 10:50 ` Richard Weinberger
0 siblings, 1 reply; 22+ messages in thread
From: Amir Goldstein @ 2017-04-11 10:20 UTC (permalink / raw)
To: Richard Weinberger
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
On Thu, Apr 6, 2017 at 3:26 PM, Richard Weinberger <richard@nod.at> wrote:
> Am 06.04.2017 um 14:09 schrieb Richard Weinberger:
>>> Were you able to make any progress? still working on this?
>>> If this is too complicated to get in for this cycle, better send a patch
>>> to disable O_TMPFILE support for ubifs and fix the problem properly on
>>> followup merge cycle.
>>> Because right now ubifs O_TMPFILE support is broken and breaks overlayfs mount.
>>
>> I have a test and currently testing it. As it looks the situation is less worse
>> than I thought first. :-)
>
> s/test/patch :)
>
Richard,
Maybe it's not my business to interfere with ubifs development and I
haven't seen your patch.
But on the face of it, it doesn't sound like fixing O_TMPFILE is a
trivial fix, so not sure
it is wise to send a patch for -rc7?...
How about sending the patch to disable O_TMPFILE for -rc7 and queuing
your fix for v4.12?
Without any patch, v4.11 is going to have a regression with overlayfs+ubifs.
Amir.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-04-11 10:20 ` Amir Goldstein
@ 2017-04-11 10:50 ` Richard Weinberger
2017-04-11 15:04 ` Amir Goldstein
0 siblings, 1 reply; 22+ messages in thread
From: Richard Weinberger @ 2017-04-11 10:50 UTC (permalink / raw)
To: Amir Goldstein
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
Hi!
Am 11.04.2017 um 12:20 schrieb Amir Goldstein:
> On Thu, Apr 6, 2017 at 3:26 PM, Richard Weinberger <richard@nod.at> wrote:
>> Am 06.04.2017 um 14:09 schrieb Richard Weinberger:
>>>> Were you able to make any progress? still working on this?
>>>> If this is too complicated to get in for this cycle, better send a patch
>>>> to disable O_TMPFILE support for ubifs and fix the problem properly on
>>>> followup merge cycle.
>>>> Because right now ubifs O_TMPFILE support is broken and breaks overlayfs mount.
>>>
>>> I have a test and currently testing it. As it looks the situation is less worse
>>> than I thought first. :-)
>>
>> s/test/patch :)
>>
>
> Richard,
>
> Maybe it's not my business to interfere with ubifs development and I
> haven't seen your patch.
>
> But on the face of it, it doesn't sound like fixing O_TMPFILE is a
> trivial fix, so not sure
> it is wise to send a patch for -rc7?...
>
> How about sending the patch to disable O_TMPFILE for -rc7 and queuing
> your fix for v4.12?
> Without any patch, v4.11 is going to have a regression with overlayfs+ubifs.
No need to panic.
I verified some stuff and my first patch does the right thing but not in a nice way,
except in oneerror patch. In will land in -rc7.
For the next merge window I prepare patches that introduce a new journal function
for handling tmpfiles.
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-04-11 10:50 ` Richard Weinberger
@ 2017-04-11 15:04 ` Amir Goldstein
2017-04-17 15:27 ` Ralph Sennhauser
0 siblings, 1 reply; 22+ messages in thread
From: Amir Goldstein @ 2017-04-11 15:04 UTC (permalink / raw)
To: Richard Weinberger
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy,
stable [v4.9],
Ralph Sennhauser
On Tue, Apr 11, 2017 at 1:50 PM, Richard Weinberger <richard@nod.at> wrote:
> Hi!
>
> Am 11.04.2017 um 12:20 schrieb Amir Goldstein:
>> On Thu, Apr 6, 2017 at 3:26 PM, Richard Weinberger <richard@nod.at> wrote:
>>> Am 06.04.2017 um 14:09 schrieb Richard Weinberger:
>>>>> Were you able to make any progress? still working on this?
>>>>> If this is too complicated to get in for this cycle, better send a patch
>>>>> to disable O_TMPFILE support for ubifs and fix the problem properly on
>>>>> followup merge cycle.
>>>>> Because right now ubifs O_TMPFILE support is broken and breaks overlayfs mount.
>>>>
>>>> I have a test and currently testing it. As it looks the situation is less worse
>>>> than I thought first. :-)
>>>
>>> s/test/patch :)
>>>
>>
>> Richard,
>>
>> Maybe it's not my business to interfere with ubifs development and I
>> haven't seen your patch.
>>
>> But on the face of it, it doesn't sound like fixing O_TMPFILE is a
>> trivial fix, so not sure
>> it is wise to send a patch for -rc7?...
>>
>> How about sending the patch to disable O_TMPFILE for -rc7 and queuing
>> your fix for v4.12?
>> Without any patch, v4.11 is going to have a regression with overlayfs+ubifs.
>
> No need to panic.
Who? me? ;-)
> I verified some stuff and my first patch does the right thing but not in a nice way,
> except in oneerror patch. In will land in -rc7.
>
That patch looks simple enough.
I though you had a more complex patch in mind.
> For the next merge window I prepare patches that introduce a new journal function
> for handling tmpfiles.
>
Thanks for the update.
Cheers,
Amir.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-04-11 15:04 ` Amir Goldstein
@ 2017-04-17 15:27 ` Ralph Sennhauser
2017-04-17 15:54 ` Richard Weinberger
0 siblings, 1 reply; 22+ messages in thread
From: Ralph Sennhauser @ 2017-04-17 15:27 UTC (permalink / raw)
To: Amir Goldstein, Richard Weinberger
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy, stable [v4.9]
On Tue, 11 Apr 2017 18:04:50 +0300
Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Apr 11, 2017 at 1:50 PM, Richard Weinberger <richard@nod.at>
> wrote:
> > Hi!
> >
> > Am 11.04.2017 um 12:20 schrieb Amir Goldstein:
> >> On Thu, Apr 6, 2017 at 3:26 PM, Richard Weinberger
> >> <richard@nod.at> wrote:
> >>> Am 06.04.2017 um 14:09 schrieb Richard Weinberger:
> >>>>> Were you able to make any progress? still working on this?
> >>>>> If this is too complicated to get in for this cycle, better
> >>>>> send a patch to disable O_TMPFILE support for ubifs and fix the
> >>>>> problem properly on followup merge cycle.
> >>>>> Because right now ubifs O_TMPFILE support is broken and breaks
> >>>>> overlayfs mount.
> >>>>
> >>>> I have a test and currently testing it. As it looks the
> >>>> situation is less worse than I thought first. :-)
> >>>
> >>> s/test/patch :)
> >>>
> >>
> >> Richard,
> >>
> >> Maybe it's not my business to interfere with ubifs development and
> >> I haven't seen your patch.
> >>
> >> But on the face of it, it doesn't sound like fixing O_TMPFILE is a
> >> trivial fix, so not sure
> >> it is wise to send a patch for -rc7?...
> >>
> >> How about sending the patch to disable O_TMPFILE for -rc7 and
> >> queuing your fix for v4.12?
> >> Without any patch, v4.11 is going to have a regression with
> >> overlayfs+ubifs.
> >
> > No need to panic.
>
> Who? me? ;-)
>
> > I verified some stuff and my first patch does the right thing but
> > not in a nice way, except in oneerror patch. In will land in -rc7.
> >
Hi Amir, Richard
Looks like the fix didn't make it into 4.11-rc7 either, isn't it time
to just disable O_TMPFILE support in ubifs for now? Giving plenty
time for the proper fix.
Thanks
Ralph
>
> That patch looks simple enough.
> I though you had a more complex patch in mind.
>
> > For the next merge window I prepare patches that introduce a new
> > journal function for handling tmpfiles.
> >
>
> Thanks for the update.
> Cheers,
> Amir.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
2017-04-17 15:27 ` Ralph Sennhauser
@ 2017-04-17 15:54 ` Richard Weinberger
0 siblings, 0 replies; 22+ messages in thread
From: Richard Weinberger @ 2017-04-17 15:54 UTC (permalink / raw)
To: Ralph Sennhauser, Amir Goldstein
Cc: Adrian Hunter, linux-mtd, linux-kernel, Artem Bityutskiy, stable [v4.9]
Am 17.04.2017 um 17:27 schrieb Ralph Sennhauser:
> Hi Amir, Richard
>
> Looks like the fix didn't make it into 4.11-rc7 either, isn't it time
> to just disable O_TMPFILE support in ubifs for now? Giving plenty
> time for the proper fix.
As I said to Amir, don't panic. I'm *very* busy right now with non-computer stuff.
My first fix is correct, I was wrong about the testing, it does the right thing,
although it was not so clear.
Except that my fix misses one error case which I wanted to push tomorrow since today
is a official holiday here in Austria.
For the next merge window I have a better approach which is better than (ab)using
ubifs_jnl_update() for O_TMPFILE.
Thanks,
//richard
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-04-17 15:54 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 8:56 [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger
2017-03-30 8:59 ` Amir Goldstein
2017-03-30 9:03 ` Richard Weinberger
2017-03-30 9:07 ` Amir Goldstein
2017-03-30 9:09 ` Richard Weinberger
2017-03-30 9:10 ` Ralph Sennhauser
2017-03-30 9:32 ` Adrian Hunter
2017-03-30 9:49 ` Richard Weinberger
2017-03-30 10:23 ` Richard Weinberger
2017-03-30 10:35 ` Amir Goldstein
2017-03-30 10:51 ` Richard Weinberger
2017-03-30 11:57 ` Adrian Hunter
2017-03-30 12:27 ` Richard Weinberger
2017-04-06 12:06 ` Amir Goldstein
2017-04-06 12:09 ` Richard Weinberger
2017-04-06 12:26 ` Richard Weinberger
2017-04-11 10:20 ` Amir Goldstein
2017-04-11 10:50 ` Richard Weinberger
2017-04-11 15:04 ` Amir Goldstein
2017-04-17 15:27 ` Ralph Sennhauser
2017-04-17 15:54 ` Richard Weinberger
2017-03-30 10:34 ` Hyunchul Lee
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).