From: Amir Goldstein <firstname.lastname@example.org>
To: Miklos Szeredi <email@example.com>
Cc: Vivek Goyal <firstname.lastname@example.org>,
Al Viro <email@example.com>,
Subject: Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode
Date: Thu, 17 May 2018 11:45:04 +0300 [thread overview]
Message-ID: <CAOQ4uxiJzdrWjkbhXE88ZUc6TSv9NTE4eRQFh6bFvBJ6xXarxA@mail.gmail.com> (raw)
On Thu, May 17, 2018 at 11:10 AM, Miklos Szeredi <firstname.lastname@example.org> wrote:
> On Thu, May 17, 2018 at 8:03 AM, Amir Goldstein <email@example.com> wrote:
>> On Tue, May 15, 2018 at 4:37 PM, Amir Goldstein <firstname.lastname@example.org> wrote:
>>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <email@example.com> wrote:
>>>> On Tue, May 15, 2018 at 01:26:09PM +0300, Amir Goldstein wrote:
>>>>> Currently, there is a small window where ovl_obtain_alias() can
>>>>> race with ovl_instantiate() and create two different overlay inodes
>>>>> with the same underlying real non-dir non-hardlink inode.
>>>>> The race requires an adversary to guess the file handle of the
>>>>> yet to be created upper inode and decode the guessed file handle
>>>>> after ovl_creat_real(), but before ovl_instantiate().
>>>>> This patch fixes the race, by using insert_inode_locked4() to add
>>>>> a newly created inode to icache.
>>>>> If the newly created inode apears to already exist in icache (hashed
>>>>> by the same real upper inode), we export this error to user instead
>>>>> of silently not hashing the new inode.
>>>> So we might return an error to user saying operation failed, but still
>>>> create file on upper. Does that sound little odd?
>>> Yes, but I don't see a better solution.
>>>> I am wondering why can't we call ovl_get_inode() in object creation
>>>> path. That should take care of race between creation path and file
>>>> handle decode and only one of the paths will get to instantiate and
>>>> initialize ovl_inode and other path will wait.
>>> I don't even want to think if what you wrote makes sense.
>>> Remember that the use case we are talking about is quite imaginary.
>>> Ensuring internal structures consistency in our code and returning
>>> error to user is the right thing to do for imaginary use cases IMO.
>> Having being forced to think about it ;-), I think using ovl_get_inode()
>> in create code does make a weird sort of sense.
> Going through the same code-path very much makes sense.
>> The reason it is weird is because we will always be throwing away
>> the new inode that we allocated in ovl_create_object().
>> AFAICS, if only reason we need to allocate new inode in
>> ovl_create_object() is to calculate i_mode with inode_init_owner()
>> and that calculation can be factored out to not need an inode.
> Not the only reason: we don't want inode allocation to fail after
> successful creation. Solution: add a preallocated inode argument to
> ovl_get_inode() and deal with allocation failure there.
IIUC that would be moving the problem to another place.
Are you suggesting that only in ENOMEM case we resort to
using the preallocated inode and inserting it safely to cache?
I'll make a variant of iget5_locked() that takes a preallocated inode
argument to use instead of alloc_inode() and will always use that
code path for creating objects.
Or maybe that is what you meant.
next prev parent reply other threads:[~2018-05-17 8:45 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-15 10:26 [PATCH v3 0/4] Overlayfs mkdir related fixes Amir Goldstein
2018-05-15 10:26 ` [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode Amir Goldstein
2018-05-15 13:23 ` Vivek Goyal
2018-05-15 13:37 ` Amir Goldstein
2018-05-16 8:34 ` Miklos Szeredi
2018-05-16 9:51 ` Amir Goldstein
2018-05-16 10:14 ` Miklos Szeredi
2018-05-16 11:03 ` Amir Goldstein
2018-05-17 6:03 ` Amir Goldstein
2018-05-17 8:10 ` Miklos Szeredi
2018-05-17 8:45 ` Amir Goldstein [this message]
2018-05-17 8:53 ` Miklos Szeredi
2018-05-17 8:58 ` Amir Goldstein
2018-05-17 9:07 ` Miklos Szeredi
2018-05-17 16:14 ` Amir Goldstein
2018-05-15 10:26 ` [PATCH v3 2/4] ovl: relax WARN_ON() real inode attributes mismatch Amir Goldstein
2018-05-15 12:48 ` Vivek Goyal
2018-05-15 12:55 ` Amir Goldstein
2018-05-16 10:29 ` Miklos Szeredi
2018-05-16 11:06 ` Amir Goldstein
2018-05-16 11:18 ` Miklos Szeredi
2018-05-16 13:46 ` Amir Goldstein
2018-05-15 10:26 ` [PATCH v3 3/4] ovl: create helper ovl_create_temp() Amir Goldstein
2018-05-16 10:41 ` Miklos Szeredi
2018-05-16 11:15 ` Amir Goldstein
2018-05-16 11:37 ` Miklos Szeredi
2018-05-15 10:26 ` [PATCH v3 4/4] ovl: make ovl_create_real() cope with vfs_mkdir() safely Amir Goldstein
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).