linux-ext4 - Re: [PATCH] fscrypt: cache decrypted symlink target in ->i

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]

Message-ID: <20190410040413.GC7140@sol.localdomain>
Date:   Tue, 9 Apr 2019 21:04:15 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [PATCH] fscrypt: cache decrypted symlink target in ->i_link

On Wed, Apr 10, 2019 at 04:44:14AM +0100, Al Viro wrote:
> On Tue, Apr 09, 2019 at 07:58:08PM -0700, Eric Biggers wrote:
> 
> > It could check a flag IOP_GET_LINK in ->i_opflags instead, so it would be the
> > same number of checks.  See patch below.
> 
> With that patch ->i_link is completely unused if ->get_link() is non-NULL,
> so you get a method call on each traversal...
> 

.get_link would be left NULL in all inode_operations that currently use
simple_get_link, then simple_get_link() would be removed.  My example patch just
changed it in ext4 as an example.

> > Benefits are that we get code that isn't actively misleading (via
> > simple_get_link() existing but actually never being called), and filesystems can
> > cache a symlink target in ->i_link if it becomes available later, i.e. if it's
> > not immediately available at iget() time.  Otherwise a filesystem-private field
> > has to be used instead.  (For fscrypt, I'd probably use fscrypt_info::ci_link.)
> 
> What's to stop you from doing just that right now?  You'd need to take
> care with barriers, but you'd need that anyway... As soon as ->i_link is set
> you'll get no more ->get_link() on that sucker, using the cached value
> from that point on.  IDGI...

1.) The VFS won't know to drop of RCU-walk mode, so waiting an RCU grace period
    before freeing the symlink target becomes mandatory.  (Which I'd like to do
    for fscrypt anyway, but doing it sanely appears to require implementing
    .destroy_inode() for ext4, f2fs, and ubifs.  I hoped I could do non-RCU mode
    as a simpler first step.)

2.) The VFS won't know to use a read memory barrier when loading i_link.
    The VFS could issue one unconditionally, but it would be unnecessary for
    regular fast symlinks.

- Eric