| 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
| ||
|
Message-ID: <BANLkTimNK8RWxz7699EdUdbWxYJvjhmerQ@mail.gmail.com>
Date: Tue, 24 May 2011 01:08:44 -0700
From: Manish Katiyar <mkatiyar@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: ext4 <linux-ext4@...r.kernel.org>, "Theodore Ts'o" <tytso@....edu>
Subject: Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to
specify gfp_mask for transaction allocation.
On Mon, May 23, 2011 at 9:41 AM, Jan Kara <jack@...e.cz> wrote:
> On Sat 21-05-11 19:43:10, Manish Katiyar wrote:
>> On Wed, May 11, 2011 at 9:04 AM, Jan Kara <jack@...e.cz> wrote:
>> > On Sun 24-04-11 17:13:18, Manish Katiyar wrote:
>> >> Update low level ext4 journal routines to pass an extra parameter
>> >> to journal allocation routines to specify whether transaction allocation
>> >> can fail or not. With this patch ext4_journal_start() can fail due to
>> >> ENOMEM. Added a new interface ext4_journal_start_tryhard() which isn't
>> >> supposed to fail and keep retrying till the allocation succeeds.
>> > As I wrote in a comment in the comment to the first patch, first just
>> > make ext4_journal_start_sb() and similar functions pass false as a part of
>> > the first patch.
>> >
>> > Then it would be better to create a new function that passes true - the
>> > name does not really matter since it will be removed later in the series
>> > but it will help the review process. You can call it
>> > ext4_journal_start_sb_enomem() or whatever. This way we keep backward
>> > compatibility because currently all call sites really expect the retry
>> > behavior.
>>
>> Hi Jan,
>>
>> Here is the updated patch incorporating your comments. This adds a new
>> function ext4_journal_start_failok and updates the ext4 code where we
>> can fail.
>>
>> This patch adds a new wrapper ext4_journal_start_failok() which
>> can fail with -ENOMEM. Update the ext4 code with this, where callers
>> are ok failing the transaction start.
> Thanks. My comments are below.
Thanks a lot Jan,
Will send the updated patch based on your comments.
>
>> Signed-off-by: Manish Katiyar <mkatiyar@...il.com>
>> ---
>> fs/ext4/acl.c | 6 +++---
>> fs/ext4/ext4_jbd2.h | 10 +++++++++-
>> fs/ext4/extents.c | 2 +-
>> fs/ext4/inode.c | 19 +++++++++++--------
>> fs/ext4/ioctl.c | 4 ++--
>> fs/ext4/migrate.c | 4 ++--
>> fs/ext4/move_extent.c | 2 +-
>> fs/ext4/namei.c | 23 +++++++++++++++--------
>> fs/ext4/super.c | 17 ++++++++++++++---
>> fs/ext4/xattr.c | 3 ++-
>> fs/jbd2/transaction.c | 4 +++-
>> 11 files changed, 63 insertions(+), 31 deletions(-)
>>
>> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
>> index 21eacd7..cdb1f51 100644
>> --- a/fs/ext4/acl.c
>> +++ b/fs/ext4/acl.c
>> @@ -350,11 +350,10 @@ ext4_acl_chmod(struct inode *inode)
>> int retries = 0;
>>
>> retry:
>> - handle = ext4_journal_start(inode,
>> + handle = ext4_journal_start_failok(inode,
>> EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
>> if (IS_ERR(handle)) {
>> error = PTR_ERR(handle);
>> - ext4_std_error(inode->i_sb, error);
> Here, you should rather do
> if (error != ENOMEM)
> ext4_std_error(inode->i_sb, error);
> We probably want to know about EIO (which is the other realistic error).
Ok.... will skip it only for -ENOMEM.
>
>> @@ -449,7 +448,8 @@ ext4_xattr_set_acl(struct dentry *dentry, const
>> char *name, const void *value,
>> acl = NULL;
>>
>> retry:
>> - handle = ext4_journal_start(inode, EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
>> + handle = ext4_journal_start_failok(inode,
>> + EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
>> if (IS_ERR(handle))
>> return PTR_ERR(handle);
>> error = ext4_set_acl(handle, inode, type, acl);
> This change is OK. But looking at the code there, we should rather do
> if (IS_ERR(handle)) {
> error = PTR_ERR(handle);
> goto release_and_out;
> }
> Can you please include this change in your other patch fixing ACL error
> handling? Thanks.
I already had fixed this as part of the earlier ACL patch that I
posted, so didn't fix it here.
>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index f2fa5e8..f7b2d4d 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3523,7 +3523,7 @@ retry:
>> int err;
>>
>> /* Credits for sb + inode write */
>> - handle = ext4_journal_start(inode, 2);
>> + handle = ext4_journal_start_failok(inode, 2);
>> if (IS_ERR(handle)) {
>> /* This is really bad luck. We've written the data
>> * but cannot extend i_size. Bail out and pretend
> Here we shouldn't fail because that will leave blocks outside EOF
> allocated. So just leave there original ext4_journal_start().
ohh okie... Actually for one of the similar patches earlier, you had
suggested that it can fail, so I followed the same. Will change it to
nofail version.
>
>> @@ -5371,7 +5372,9 @@ int ext4_setattr(struct dentry *dentry, struct
>> iattr *attr)
>> rc = ext4_acl_chmod(inode);
>>
>> err_out:
>> - ext4_std_error(inode->i_sb, error);
>> + if (error != -ENOMEM) {
>> + ext4_std_error(inode->i_sb, error);
>> + }
> No need for braces here...
ok.
>
>> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
>> index 92816b4..8870746 100644
>> --- a/fs/ext4/migrate.c
>> +++ b/fs/ext4/migrate.c
>> @@ -533,7 +533,7 @@ int ext4_ext_migrate(struct inode *inode)
>> ext4_set_inode_state(inode, EXT4_STATE_EXT_MIGRATE);
>> up_read((&EXT4_I(inode)->i_data_sem));
>>
>> - handle = ext4_journal_start(inode, 1);
>> + handle = ext4_journal_start_failok(inode, 1);
>> if (IS_ERR(handle)) {
>> /*
>> * It is impossible to update on-disk structures without
> Here we should better not fail because we have inode on orphan list and
> need to eventually remove it. So just keep old ext4_journal_start().
ok.
>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 4e4c17f..2d57a57 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -247,7 +247,8 @@ static void ext4_put_nojournal(handle_t *handle)
>> * ext4 prevents a new handle from being started by s_frozen, which
>> * is in an upper layer.
>> */
>> -handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
>> +static handle_t *ext4_journal_start_sb_int(struct super_block *sb,
>> + int nblocks, bool errok)
> Maybe __ext4_journal_start_sb() would be a more usual name...
>
>> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
>> index b5c2550..3453c29 100644
>> --- a/fs/jbd2/transaction.c
>> +++ b/fs/jbd2/transaction.c
>> @@ -308,6 +308,8 @@ static handle_t *new_handle(int nblocks)
>> * handle_t *jbd2_journal_start() - Obtain a new handle.
>> * @journal: Journal to start transaction on.
>> * @nblocks: number of block buffer we might modify
>> + * @errok : True if the transaction allocation can fail
>> + * with ENOMEM.
>> *
>> * We make sure that the transaction can guarantee at least nblocks of
>> * modified buffers in the log. We block until the log can guarantee
> Move this to the patch adding the parameter...
Will do.
>
>> @@ -338,7 +340,7 @@ handle_t *jbd2_journal_start(journal_t *journal,
>> int nblocks, bool errok)
>>
>> current->journal_info = handle;
>>
>> - err = start_this_handle(journal, handle, GFP_NOFS);
>> + err = start_this_handle(journal, handle, errok ? GFP_KERNEL : GFP_NOFS);
>> if (err < 0) {
>> jbd2_free_handle(handle);
>> current->journal_info = NULL;
> This is probably just a leftover from some previous version?
Actually no. I added this as part of this patch. So do I actually
switch the gfp_mask in the last patch of the series ?
--
Thanks -
Manish
--
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
Powered by blists - more mailing lists