Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1612748 > unrolled thread

[PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()

Started byRichard Weinberger <richard@nod.at>
First post2017-03-30 11:00 +0200
Last post2017-03-30 12:40 +0200
Articles 17 — 5 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-03-30 11:00 +0200
    Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Amir Goldstein <amir73il@gmail.com> - 2017-03-30 11:10 +0200
      Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-03-30 11:20 +0200
    Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Amir Goldstein <amir73il@gmail.com> - 2017-03-30 11:10 +0200
      Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-03-30 11:20 +0200
      Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-30 11:20 +0200
    Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Adrian Hunter <adrian.hunter@intel.com> - 2017-03-30 11:40 +0200
      Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-03-30 11:50 +0200
        Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-03-30 12:30 +0200
          Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Amir Goldstein <amir73il@gmail.com> - 2017-03-30 12:40 +0200
            Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-03-30 13:00 +0200
          Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Adrian Hunter <adrian.hunter@intel.com> - 2017-03-30 14:10 +0200
            Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-03-30 14:30 +0200
              Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-04-06 14:10 +0200
                Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Richard Weinberger <richard@nod.at> - 2017-04-06 14:30 +0200
              Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Amir Goldstein <amir73il@gmail.com> - 2017-04-06 14:10 +0200
    Re: [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link() Hyunchul Lee <hyc.lee@gmail.com> - 2017-03-30 12:40 +0200

#1612748 — [PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()

FromRichard Weinberger <richard@nod.at>
Date2017-03-30 11:00 +0200
Subject[PATCH] ubifs: Fix O_TMPFILE corner case in ubifs_link()
Message-ID<tqE5Y-3Mi-15@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1612758

FromAmir Goldstein <amir73il@gmail.com>
Date2017-03-30 11:10 +0200
Message-ID<tqEfE-451-23@gated-at.bofh.it>
In reply to#1612748
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 ;-)

[toc] | [prev] | [next] | [standalone]

#1612767

FromRichard Weinberger <richard@nod.at>
Date2017-03-30 11:20 +0200
Message-ID<tqEpj-48C-9@gated-at.bofh.it>
In reply to#1612758
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

[toc] | [prev] | [next] | [standalone]

#1612763

FromAmir Goldstein <amir73il@gmail.com>
Date2017-03-30 11:10 +0200
Message-ID<tqEfE-451-25@gated-at.bofh.it>
In reply to#1612748
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
>

[toc] | [prev] | [next] | [standalone]

#1612769

FromRichard Weinberger <richard@nod.at>
Date2017-03-30 11:20 +0200
Message-ID<tqEfE-451-27@gated-at.bofh.it>
In reply to#1612763
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

[toc] | [prev] | [next] | [standalone]

#1612771

FromRalph Sennhauser <ralph.sennhauser@gmail.com>
Date2017-03-30 11:20 +0200
Message-ID<tqEpk-48C-13@gated-at.bofh.it>
In reply to#1612763
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
> >

[toc] | [prev] | [next] | [standalone]

#1612789

FromAdrian Hunter <adrian.hunter@intel.com>
Date2017-03-30 11:40 +0200
Message-ID<tqEIF-4h2-3@gated-at.bofh.it>
In reply to#1612748
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?

[toc] | [prev] | [next] | [standalone]

#1612800

FromRichard Weinberger <richard@nod.at>
Date2017-03-30 11:50 +0200
Message-ID<tqESm-4kQ-23@gated-at.bofh.it>
In reply to#1612789
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

[toc] | [prev] | [next] | [standalone]

#1612895

FromRichard Weinberger <richard@nod.at>
Date2017-03-30 12:30 +0200
Message-ID<tqFv4-4UX-25@gated-at.bofh.it>
In reply to#1612800
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

[toc] | [prev] | [next] | [standalone]

#1612912

FromAmir Goldstein <amir73il@gmail.com>
Date2017-03-30 12:40 +0200
Message-ID<tqFEK-4YK-13@gated-at.bofh.it>
In reply to#1612895
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..

[toc] | [prev] | [next] | [standalone]

#1612952

FromRichard Weinberger <richard@nod.at>
Date2017-03-30 13:00 +0200
Message-ID<tqFY5-59J-7@gated-at.bofh.it>
In reply to#1612912
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

[toc] | [prev] | [next] | [standalone]

#1612999

FromAdrian Hunter <adrian.hunter@intel.com>
Date2017-03-30 14:10 +0200
Message-ID<tqH3P-65b-9@gated-at.bofh.it>
In reply to#1612895
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.

[toc] | [prev] | [next] | [standalone]

#1613022

FromRichard Weinberger <richard@nod.at>
Date2017-03-30 14:30 +0200
Message-ID<tqHnb-6iA-15@gated-at.bofh.it>
In reply to#1612999
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

[toc] | [prev] | [next] | [standalone]

#1617905

FromRichard Weinberger <richard@nod.at>
Date2017-04-06 14:10 +0200
Message-ID<tteoF-8jD-1@gated-at.bofh.it>
In reply to#1613022
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

[toc] | [prev] | [next] | [standalone]

#1617934

FromRichard Weinberger <richard@nod.at>
Date2017-04-06 14:30 +0200
Message-ID<tteI2-8tl-27@gated-at.bofh.it>
In reply to#1617905
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

[toc] | [prev] | [next] | [standalone]

#1617913

FromAmir Goldstein <amir73il@gmail.com>
Date2017-04-06 14:10 +0200
Message-ID<tteoF-8jD-3@gated-at.bofh.it>
In reply to#1613022
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.

[toc] | [prev] | [next] | [standalone]

#1612924

FromHyunchul Lee <hyc.lee@gmail.com>
Date2017-03-30 12:40 +0200
Message-ID<tqFEL-4YK-45@gated-at.bofh.it>
In reply to#1612748

[Multipart message — attachments visible in raw view] — view raw

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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web