linux-kernel - Re: [PATCH v2] ext4: have ext4_xattr_set

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:   Fri, 30 Jun 2017 12:36:51 -0700
From:   Tahsin Erdogan <tahsin@...gle.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     "Theodore Ts'o" <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] ext4: have ext4_xattr_set_handle() allocate journal credits

> One problem with this approach is that restarting the transaction handle will
> make the xattr update non-atomic, which could be a real problem for some
> workloads.  For example, ACLs or SELinux or fscrypt xattrs being added in
> a separate transaction from file creation, or being modified (delete in a
> separate transaction from add) and then lost completely if the system crashes
> before the second transaction is committed.

Agreed.

> It isn't clear to me why using the current helper function to precompute the
> required transaction credits doesn't get this right in the first place?  It
> would just need to add the xattr credits to the original journal handle?

An example code path is this:

ext4_mkdir()
  ext4_new_inode_start_handle()
    __ext4_new_inode()   <<== transaction handle is started here
      ext4_init_acl()
        __ext4_set_acl()
          ext4_xattr_set_handle()

In this case, __ext4_new_inode() needs to figure out all journal
credits needed including the ones for ext4_xattr_set_handle(). This is
a few levels deep so reaching out to ext4_xattr_set_credits() with the
right parameters is where the complexity lies.