linux-ext4 - Re: ext4_orphan_del() sleeps in non-journal mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 15 Sep 2012 15:51:21 -0700
From:	Anatol Pomozov <anatol.pomozov@...il.com>
To:	Dmitry Monakhov <dmonakhov@...nvz.org>
Cc:	linux-ext4@...r.kernel.org, "Theodore Ts'o" <tytso@....edu>
Subject: Re: ext4_orphan_del() sleeps in non-journal mode

Hi

On Sat, Sep 15, 2012 at 3:28 PM, Anatol Pomozov
<anatol.pomozov@...il.com> wrote:
> D'oh, sorry.
>
> I did not realize that it is our local modification that calls
> ext4_orphan_del(NULL) from ext4_end_io_dio. So please ignore the issue
> "ext4_orphan_del sleeps in softirq context".
>
> But the other more general issue "ext4_orphan_del sleeps in no-journal
> mode" still applies. As Dmitry mentioned in this commit 3d287de3b828
> such sleep might degrade performance. In no-journal mode we do not
> need to manipulate with i_orphan list and no reason to take the mutex.
>
> I checked all ext4_orphan_del(NULL,...) usages and some of them look
> good for me, e.g.
>
>                 handle = ext4_journal_start(inode, 2);
>                 if (IS_ERR(handle)) {
>                         if (inode->i_nlink)
>                                 ext4_orphan_del(NULL, inode);
>
> In this example we take handle and important thing to note here is
> that IS_ERR(handle) can be true only in journal mode when starting
> journal failed. In non-journal mode ext4_journal_start() *always*
> returns a fake handle that is non-error (see ext4_get_nojournal). So
> the example above never sleeps in ext4_orphan_del().
>
> But there are other examples where sleep might happen (at current HEAD
> 3f0c3c8fe):
>
> inode.c:281
> inode.c:956
> inode.c:1069
> inode.c:1111
> inode.c:1177
>
> I think we need to fix these 5 places. In all other cases we check
> IS_ERR(handle) and thus do not call ext4_orphan_del() in no-journal
> mode.

Actually inode.c:1177 is also fine as we check ext4_handle_valid() in
this function above, thus this codepath can be never called in
no-journal mode. The other 4 places (and plus my local code) have
potential issues.

I am still trying to understand what is the semantics of
"ext4_orphand_del(NULL,...)". Does NULL mean

a) journaling is enabled, an error happened and we do not have a valid
handle anymore but we need to remove an added inode from orphan list.
In this case we should either check "EXT4_SB(sb)->s_journal" *before*
calling ext4_orphand_del, or introduce a flag that indicated the inode
was successfully added (like Dmitry made in 3d287de3).

b) and error happened and we need to remove inode from orphan list. In
this case ext4_orphand_del() is responsible for checking whether
journal is enabled.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html