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

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

[Richard Weinberger]

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

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

[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 >

> 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
>

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

[Richard Weinberger]

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

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

[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).

I would also add # v4.9 hint to stable tag, just to be nice ;-)

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

[Richard Weinberger]

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

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

[Ralph Sennhauser]

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
> >  

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

[Adrian Hunter]

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?

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

[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. :-)

Thanks,
//richard

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

[Richard Weinberger]

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

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

[Amir Goldstein]

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..

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

[Richard Weinberger]

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

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

[Adrian Hunter]

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.

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

[Richard Weinberger]

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

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

[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.

Amir.

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

[Richard Weinberger]

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

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

[Richard Weinberger]

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

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

[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.

Amir.

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

[Richard Weinberger]

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

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

[Amir Goldstein]

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.

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

[Ralph Sennhauser]

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.

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

[Richard Weinberger]

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

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

[Hyunchul Lee]

[-- 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);