linux-ext4 - Re: Errors reported by Coverity in ext3

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: <18730dc50904210201h454e9790y434e35c364dbb9d1@mail.gmail.com>
Date:	Tue, 21 Apr 2009 12:01:55 +0300
From:	Amir Goldor <amir73il@...rs.sourceforge.net>
To:	Andreas Dilger <adilger@....com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: Errors reported by Coverity in ext3

Thanks for your corrections.

Attached is a new patch against kernel 2.6.29.1.
I applied most of your corrections, see my comments inline.
Please review take #2.

Thanks,
Amir.

On Tue, Apr 21, 2009 at 1:14 AM, Andreas Dilger <adilger@....com> wrote:
>
> Thanks for the patch, unfortunately it is a NACK, since there are a few
> bugs in the error handling, and some additional cleanups that can be done.
>
> More comments inline.
>
>> @@ -2302,7 +2310,9 @@ static int ext3_rename (struct inode * o
>>               BUFFER_TRACE(new_bh, "get write access");
>> -             ext3_journal_get_write_access(handle, new_bh);
>> +             retval = ext3_journal_get_write_access(handle, new_bh);
>> +             if (retval)
>> +                     goto end_rename;
>
> Similarly, this also needs a "brelse(new_bh)" before "goto end_rename".

brelse(new_bh) as well as brelse(dir_bh) are called on end_rename.

>
>> @@ -2360,7 +2370,14 @@ static int ext3_rename (struct inode * o
>>       ext3_update_dx_flag(old_dir);
>>       if (dir_bh) {
>>               BUFFER_TRACE(dir_bh, "get_write_access");
>> +             retval = ext3_journal_get_write_access(handle, dir_bh);
>> +             if (retval) {
>> +                     ext3_warning(old_dir->i_sb, "ext3_rename",
>> +                                     "Updating new directory (%lu) parent link, %d, error=%d",
>> +                                     new_dir->i_ino, new_dir->i_nlink, retval);
>> +             }
>> +     }
>
> At this point, if we cannot get write access to the directory buffer, it
> is really a bit too late to do much about it.  It may make more sense to
> instead call ext3_journal_get_write_access() right after ext3_bread() is
> called dor this buffer, so that the error can be checked and the rename
> aborted before any changes are made.
>

good idea. I moved it up.

> Note also that there are some places where ext3_journal_dirty_metadata()
> are called in these same code paths, but the error is not checked by the
> caller.
>

true, but as with the case above, handling errors before the changes
are made is much easier,
and I am only aiming for 'best effort' in this patch.

Download attachment "ext3-namei-check-jbd-errors-2.6.29.1.patch" of type "application/octet-stream" (2552 bytes)