linux-ext4 - Re: [PATCH] ext4: directory blocks must be treated as metadata by ext4

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: <6601abe90911151548w5abb326ag637718d6a800d29b@mail.gmail.com>
Date:	Sun, 15 Nov 2009 15:48:10 -0800
From:	Curt Wohlgemuth <curtw@...gle.com>
To:	Theodore Tso <tytso@....edu>
Cc:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: directory blocks must be treated as metadata by 
	ext4_forget()

On Sun, Nov 15, 2009 at 12:43 PM, Theodore Tso <tytso@....edu> wrote:
> On Sun, Nov 15, 2009 at 12:34:48PM +0530, Aneesh Kumar K.V wrote:
>>
>> I guess we need to make sure we call ext4_forget with correct
>> is_metadata values. I did the below patch. The xattr changes in the
>> patch should be split as a separate one.  I am not sure why we do a
>> get_bh there.
>
> It doesn't hurt to call ext4_forget() with the correct values, but I
> figured it was easier just to make ext4_forget() DTRT thing by
> checking the inode type since it has access to i_mode.  My patch
> didn't take into account symlinks, though.   Good catch on your part.
>
>> Another question i have is, do we actually supporting freeing
>> directory blocks when we delete directory entries ? I remember
>> reading we don't have support for that.
>
> No, we don't.
>
>> So may be Curt is not
>> seeing the ext4_forget being called because he is trying delete of
>> directory entries. I guess he will have to do a rmdir directory to
>> see the directory blocks freed.
>
> I'm assuming the problem that Curt was seeing was due to directories
> being deleted, and the blocks getting reused immediately afterwards
> for data blocks.  I'm guessing the right was done via direct I/O,
> which means it would have been posted right away, and somehow the
> dirty buffer head some managed to not get forgotten via bforget().  In
> the non-journal case, I don't see how that could happen, but I must be
> missing something with the code paths.  My experiments show that
> ext4_forget() is getting called, but apparently somehow bforget() must
> be getting called after that point.

Yes, I'm also assuming that the problem is with deleting directories.
And yes, DIO is used for the 8MB files that are being corrupted.

I'm not sure how I missed the call to ext4_forget() in
ext4_remove_blocks() and ext4_clear_blocks(), but I did -- or at least
I didn't realize they were called for all data blocks being freed.
Thanks.

As to why this happened on our systems where no journal is being used:
 I believe these were running older kernels that didn't have the
patches in c7acb4c16646943180bd221c167a077e0a084f9c, and hence weren't
calling bforget() properly.

Thanks,
Curt

>
>> If you think the changes are correct i will send proper patches with s-o-b
>
> I already have a patch in the patch queue, and I'll just update it to
> include checking for S_ISLNK(inode->i_mode).  I suppose I can add your
> change to set is_metadata in ext4_remove_blocks(), but that only
> handles the extents case.  The direct/indirect mapped case also has a
> similar issue, which is why decided it was most straightforward to fix
> it in ext4_forget().
>
>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>> index fed5b01..3c93a9a 100644
>> --- a/fs/ext4/xattr.c
>> +++ b/fs/ext4/xattr.c
>> @@ -482,9 +482,8 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
>>               ea_bdebug(bh, "refcount now=0; freeing");
>>               if (ce)
>>                       mb_cache_entry_free(ce);
>> -             ext4_free_blocks(handle, inode, bh->b_blocknr, 1, 1);
>> -             get_bh(bh);
>>               ext4_forget(handle, 1, inode, bh, bh->b_blocknr);
>> +             ext4_free_blocks(handle, inode, bh->b_blocknr, 1, 1);
>>       } else {
>>               le32_add_cpu(&BHDR(bh)->h_refcount, -1);
>>               error = ext4_handle_dirty_metadata(handle, inode, bh);
>
> This change isn't needed, as you pointed out in a later e-mail,
> ext4_xattr_release_block() isn't supposed to change the refcount of
> the buffer_head; it is brelse'ed by its caller.
>
>                                        - Ted
>
--
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