Re: Error Propagation bugs in ext4

public inbox for linux-ext4@vger.kernel.org
 help / color / mirror / Atom feed

* Re: Error Propagation bugs in ext4
       [not found] <adb1f7050810311819s3ea0b8f1mbb4239b108f34c4e@mail.gmail.com>
@ 2008-11-22 17:39 ` Andreas Dilger
       [not found] ` <adb1f7050811041014r5be96821ie938c313aaf48334@mail.gmail.com>
  1 sibling, 0 replies; 3+ messages in thread
From: Andreas Dilger @ 2008-11-22 17:39 UTC (permalink / raw)
  To: Cindy Rubio Gonz�lez; +Cc: linux-ext4, adilger, cmm, Ben Liblit

On Oct 31, 2008  20:19 -0500, Cindy Rubio Gonz�lez wrote:
> I am a graduate student at the University of Wisconsin-Madison. My
> advisor (Prof. Ben Liblit) and I have been working on performing
> static analysis to find error propagation bugs in Linux file system
> implementations. ext4 is one of our case studies. We consider three
> general scenarios in which unchecked error are commonly lost: the
> variable holding the unchecked error value (1) is overwritten with a
> new value, (2) goes out of scope, or (3) is returned by a function but
> not saved by the caller. Our tool produces detailed witness traces
> that illustrate each bug reported.

I apologize for not getting back to you on this sooner, I've been very
busy and barely able to keep up on my own work for the past few weeks.

> Slice:
> fs/jbd2/transaction.c:105:"ret" receives an error from "TENTATIVE_ENOMEM"
> fs/jbd2/transaction.c:241: an unchecked error may be returned
> fs/jbd2/transaction.c:427:"ret" receives an error from function
> "start_this_handle"
> fs/jbd2/transaction.c:428: an unchecked error may be returned
> fs/ext4/ext4_jbd2.h:188:"cabs2cil_" receives an error from function
> "jbd2_journal_restart"
> fs/ext4/ext4_jbd2.h:188: an unchecked error may be returned
> fs/ext4/extents.c:104:"cabs2cil_" receives an error from function
> "ext4_journal_restart"
> fs/ext4/extents.c:104: an unchecked error may be returned
> fs/ext4/extents.c:1933:"err" receives an error from function
> "ext4_ext_journal_restart"
> fs/ext4/extents.c:1980: an unchecked error may be returned

In this particular example, it appears that there is a logic error
in the simulator.  This is the fragment of code that is in question:

2037     while (i >= 0 && err == 0) {
2038             if (i == depth) {
2039                     /* this is leaf block */
2040                     err = ext4_ext_rm_leaf(handle, inode, path, start);
2041                     /* root level has p_bh == NULL, brelse() eats this */
2042                     brelse(path[i].p_bh);
2043                     path[i].p_bh = NULL;
2044                     i--;
2045                     continue;
2046             }

> fs/ext4/extents.c:2040:"err" receives an error from "ext4_ext_rm_leaf"
2040                     err = ext4_ext_rm_leaf(handle, inode, path, start);
> fs/ext4/extents.c:2042:"err" may have an unchecked error
2042                     brelse(path[i].p_bh);
> include/linux/buffer_head.h:270:"err" may have an unchecked error
2043                     path[i].p_bh = NULL;
> fs/ext4/extents.c:2043:"err" may have an unchecked error
2043                     path[i].p_bh = NULL;
> fs/ext4/extents.c:2044:"err" may have an unchecked error
2044                     i--;
> fs/ext4/extents.c:2045:"err" may have an unchecked error
2045                     continue;
> fs/ext4/extents.c:2037:"err" may have an unchecked error
2037     while (i >= 0 && err == 0) {
> fs/ext4/extents.c:2037:"err" may have an unchecked error
> fs/ext4/extents.c:2037:"err" may have an unchecked error
> fs/ext4/extents.c:2038:"err" may have an unchecked error
:
:
> fs/ext4/extents.c:2075:"err" may have an unchecked error
2075                         bh = sb_bread(sb, idx_pblock(path[i].p_idx));
> fs/ext4/extents.c:2076:"err" may have an unchecked error
2076                         if (!bh) {
> fs/ext4/extents.c:2078: overwriting potential non-tentative unchecked
> error in "err"
2078                                 err = -EIO;
> ====

It is clear that this loop should terminate because "err == 0" is now
false.  Not only did the simulator fail to detect that the error in
"err" was checked at this point, the simulator execution flow incorrectly
shows the loop continuing until line 2078 where "err" is ostensibly
overwritten, but that is not what would actually happen.


I haven't looked at the other failure cases, because I suspect there
may be some similar false positives and I'd rather that the software
detect them than me.

That said, I'm a HUGE fan of static code analysis, and I'd be happy if
you could re-run your analysis and forward the results to the list again.
I think this is an excellent way of detecting bugs that happen rarely
enough that there are no good reproduction cases, but likely hit users
on occasion and are otherwise impossible to diagnose.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 3+ messages in thread

[parent not found: <adb1f7050811041014r5be96821ie938c313aaf48334@mail.gmail.com>]

* Re: Error Propagation bugs in ext4
       [not found] ` <adb1f7050811041014r5be96821ie938c313aaf48334@mail.gmail.com>
@ 2008-11-11 15:19   ` Cindy Rubio González
  2008-11-22 17:39   ` Andreas Dilger
  1 sibling, 0 replies; 3+ messages in thread
From: Cindy Rubio González @ 2008-11-11 15:19 UTC (permalink / raw)
  To: linux-ext4; +Cc: adilger, cmm

Hi everyone,

I am Ph.D. student at the University of Wisconsin-Madison and I
submitted some error-propagation bug reports a few days back. We were
wondering whether anyone has had the chance to look at some of these
reports. We are submitting this work for conference publication this
Friday, November 14th. It would be really great to hear your
comments!! We are really excited to know how many of the reports are
true bugs and how serious these bugs are (even if the bugs have not
been fixed yet). Our deadline is close and we understand this
information may not be available yet, but please feel free to send us
any other general comments you may have.

Please let me know if you have any questions. Thank you!
Cindy

On Tue, Nov 4, 2008 at 12:14 PM, Cindy Rubio González
<crubio@cs.wisc.edu> wrote:
> Hi everyone,
>
> This is a follow-up to the e-mail sent a couple of days ago reporting
> some error-propagation bugs found using static analysis. We have 100
> additional unsaved-error reports:
>
> Total number of reports: 135
> Overwritten errors: 3
> Unsaved errors: 1
> Out-of-scope errors: 131
>
> I have attached the new ext4.summ and ext4.complete. We have manually
> inspected each unsaved-error report and we are now providing more
> information hoping it is easier and faster for you to determine
> whether they are indeed bugs. We have grouped unsaved-error reports by
> callee in the ext4.summ file. For example, you can find:
>
> mpage_da_submit_io
> fs/ext4/inode.c:1903: potential non-tentative unchecked error is not saved
> fs/ext4/inode.c:1946: potential non-tentative unchecked error is not saved
> fs/ext4/inode.c:2050: potential non-tentative unchecked error is not saved
>
> Here there are three calls to mpage_da_submit_io where the error code
> returned is not saved.
>
> We have found that there are functions for which half of the callers
> save the error whereas the other half don't. This implies that the
> returned error code is somewhat important. However, we also know that
> there are cases where it is "okay" to neglect the error code. An
> example of this big inconsistency is the function "ext4_journal_stop".
>
> It would be great to know what is your opinion on this matter.
> a) It is so common such that this type of inconsistency will always be
> present in the future?
> b) It is bad. All callers should check the returned error code
> regardless of being in the exit path?
>
> Please let us know if you have any questions. It would be great if you
> could mark those reports found to be true bugs. This would help us to
> understand the sources of false positives and improve our tool. Also,
> it would be great to get any feedback, even if the bugs have not been
> fixed yet.
>
> Thank you very much for your time,
> Cindy
>
>
> On Fri, Oct 31, 2008 at 7:19 PM, Cindy Rubio González
> <crubio@cs.wisc.edu> wrote:
>> Hi,
>>
>> I am a graduate student at the University of Wisconsin-Madison. My
>> advisor (Prof. Ben Liblit) and I have been working on performing
>> static analysis to find error propagation bugs in Linux file system
>> implementations. ext4 is one of our case studies. We consider three
>> general scenarios in which unchecked error are commonly lost: the
>> variable holding the unchecked error value (1) is overwritten with a
>> new value, (2) goes out of scope, or (3) is returned by a function but
>> not saved by the caller. Our tool produces detailed witness traces
>> that illustrate each bug reported.
>>
>> We have results for Linux 2.6.27. Our tool has produced 35 bug
>> reports for ext4:
>>
>> Total number of reports: 35
>> Overwritten errors: 3
>> Unsaved errors: 1
>> Out-of-scope errors: 31
>>
>> Attached are two files with the results.
>>
>> ext4.summ includes the file and line number at
>> which an error may be dropped (and the classification: overwritten,
>> out of scope or unsaved).
>>
>> Example:
>> fs/ext4/extents.c:2078: overwriting potential non-tentative unchecked
>> error in "err"
>>
>> ext4.complete - this file contains traces for each of the bugs
>> included in ext4.summ. There is a full trace and a slice (a trace
>> including only those lines relevant to the propagation of the error).
>> The full trace and slice for each bug are separated by "====".
>>
>> Example:
>> Full trace:
>> fs/jbd2/transaction.c:105:"ret" receives an error from "TENTATIVE_ENOMEM"
>> fs/jbd2/transaction.c:106:"ret" may have an unchecked error
>> fs/jbd2/transaction.c:239:"ret" may have an unchecked error
>> fs/jbd2/transaction.c:239:"ret" may have an unchecked error
>> fs/jbd2/transaction.c:239:"ret" may have an unchecked error
>> fs/jbd2/transaction.c:239:"ret" may have an unchecked error
>> fs/jbd2/transaction.c:241: an unchecked error may be returned
>> fs/jbd2/transaction.c:427:"ret" receives an error from function
>> "start_this_handle"
>> fs/jbd2/transaction.c:428: an unchecked error may be returned
>> fs/ext4/ext4_jbd2.h:188:"cabs2cil_" receives an error from function
>> "jbd2_journal_restart"
>> fs/ext4/ext4_jbd2.h:188: an unchecked error may be returned
>> fs/ext4/extents.c:104:"cabs2cil_" receives an error from function
>> "ext4_journal_restart"
>> fs/ext4/extents.c:104: an unchecked error may be returned
>> fs/ext4/extents.c:1933:"err" receives an error from function
>> "ext4_ext_journal_restart"
>> fs/ext4/extents.c:1934:"err" may have an unchecked error
>> fs/ext4/extents.c:1935:"err" may have an unchecked error
>> fs/ext4/extents.c:1980: an unchecked error may be returned
>> fs/ext4/extents.c:2040:"err" receives an error from function "ext4_ext_rm_leaf"
>> fs/ext4/extents.c:2042:"err" may have an unchecked error
>> include/linux/buffer_head.h:270:"err" may have an unchecked error
>> fs/ext4/extents.c:2043:"err" may have an unchecked error
>> fs/ext4/extents.c:2044:"err" may have an unchecked error
>> fs/ext4/extents.c:2045:"err" may have an unchecked error
>> fs/ext4/extents.c:2037:"err" may have an unchecked error
>> fs/ext4/extents.c:2037:"err" may have an unchecked error
>> fs/ext4/extents.c:2037:"err" may have an unchecked error
>> fs/ext4/extents.c:2038:"err" may have an unchecked error
>> fs/ext4/extents.c:2049:"err" may have an unchecked error
>> fs/ext4/extents.c:2054:"err" may have an unchecked error
>> fs/ext4/extents.c:2056:"err" may have an unchecked error
>> fs/ext4/extents.c:2057:"err" may have an unchecked error
>> fs/ext4/extents.c:2069:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1992:"err" may have an unchecked error
>> fs/ext4/extents.c:1999:"err" may have an unchecked error
>> fs/ext4/extents.c:2001:"err" may have an unchecked error
>> fs/ext4/extents.c:2069:"err" may have an unchecked error
>> fs/ext4/extents.c:2069:"err" may have an unchecked error
>> fs/ext4/extents.c:2074:"err" may have an unchecked error
>> fs/ext4/extents.c:2074:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> fs/ext4/extents.c:68:"err" may have an unchecked error
>> fs/ext4/extents.c:69:"err" may have an unchecked error
>> fs/ext4/extents.c:70:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> include/linux/buffer_head.h:281:"err" may have an unchecked error
>> fs/buffer.c:1443:"err" may have an unchecked error
>> include/linux/buffer_head.h:281:"err" may have an unchecked error
>> include/linux/buffer_head.h:281:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> fs/ext4/extents.c:2076:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2081:"err" may have an unchecked error
>> fs/ext4/extents.c:2085:"err" may have an unchecked error
>> fs/ext4/ext4_extents.h:176:"err" may have an unchecked error
>> fs/ext4/extents.c:2085:"err" may have an unchecked error
>> fs/ext4/extents.c:2085:"err" may have an unchecked error
>> fs/ext4/extents.c:2085:"err" may have an unchecked error
>> fs/ext4/extents.c:2085:"err" may have an unchecked error
>> fs/ext4/extents.c:2085:"err" may have an unchecked error
>> fs/ext4/extents.c:2090:"err" may have an unchecked error
>> fs/ext4/extents.c:2094:"err" may have an unchecked error
>> fs/ext4/extents.c:2095:"err" may have an unchecked error
>> fs/ext4/extents.c:2037:"err" may have an unchecked error
>> fs/ext4/extents.c:2037:"err" may have an unchecked error
>> fs/ext4/extents.c:2037:"err" may have an unchecked error
>> fs/ext4/extents.c:2038:"err" may have an unchecked error
>> fs/ext4/extents.c:2049:"err" may have an unchecked error
>> fs/ext4/extents.c:2054:"err" may have an unchecked error
>> fs/ext4/extents.c:2056:"err" may have an unchecked error
>> fs/ext4/extents.c:2057:"err" may have an unchecked error
>> fs/ext4/extents.c:2069:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1990:"err" may have an unchecked error
>> fs/ext4/extents.c:1992:"err" may have an unchecked error
>> fs/ext4/extents.c:1999:"err" may have an unchecked error
>> fs/ext4/extents.c:2001:"err" may have an unchecked error
>> fs/ext4/extents.c:2069:"err" may have an unchecked error
>> fs/ext4/extents.c:2069:"err" may have an unchecked error
>> fs/ext4/extents.c:2074:"err" may have an unchecked error
>> fs/ext4/extents.c:2074:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> fs/ext4/extents.c:68:"err" may have an unchecked error
>> fs/ext4/extents.c:69:"err" may have an unchecked error
>> fs/ext4/extents.c:70:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> include/linux/buffer_head.h:281:"err" may have an unchecked error
>> fs/buffer.c:1443:"err" may have an unchecked error
>> include/linux/buffer_head.h:281:"err" may have an unchecked error
>> include/linux/buffer_head.h:281:"err" may have an unchecked error
>> fs/ext4/extents.c:2075:"err" may have an unchecked error
>> fs/ext4/extents.c:2076:"err" may have an unchecked error
>> fs/ext4/extents.c:2076:"err" may have an unchecked error
>> fs/ext4/extents.c:2078: overwriting potential non-tentative unchecked
>> error in "err"
>>
>> Slice:
>> fs/jbd2/transaction.c:105:"ret" receives an error from "TENTATIVE_ENOMEM"
>> fs/jbd2/transaction.c:241: an unchecked error may be returned
>> fs/jbd2/transaction.c:427:"ret" receives an error from function
>> "start_this_handle"
>> fs/jbd2/transaction.c:428: an unchecked error may be returned
>> fs/ext4/ext4_jbd2.h:188:"cabs2cil_" receives an error from function
>> "jbd2_journal_restart"
>> fs/ext4/ext4_jbd2.h:188: an unchecked error may be returned
>> fs/ext4/extents.c:104:"cabs2cil_" receives an error from function
>> "ext4_journal_restart"
>> fs/ext4/extents.c:104: an unchecked error may be returned
>> fs/ext4/extents.c:1933:"err" receives an error from function
>> "ext4_ext_journal_restart"
>> fs/ext4/extents.c:1980: an unchecked error may be returned
>> fs/ext4/extents.c:2040:"err" receives an error from function "ext4_ext_rm_leaf"
>> fs/ext4/extents.c:2078: overwriting potential non-tentative unchecked
>> error in "err"
>> ====
>>
>> Any feedback will be greatly appreciated (we will be submitting this
>> work for conference publication in two weeks). Please let us know if
>> you
>> have any questions.
>>
>> Thank you very much for your time!
>> Cindy
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Error Propagation bugs in ext4
       [not found] ` <adb1f7050811041014r5be96821ie938c313aaf48334@mail.gmail.com>
  2008-11-11 15:19   ` Cindy Rubio González
@ 2008-11-22 17:39   ` Andreas Dilger
  1 sibling, 0 replies; 3+ messages in thread
From: Andreas Dilger @ 2008-11-22 17:39 UTC (permalink / raw)
  To: Cindy Rubio Gonz�lez; +Cc: linux-ext4, cmm, Ben Liblit

Again,
thanks for sending these results to the list.  It is unfortunate that
nobody on the list was able to look through the results until after your
paper submission.

On Nov 04, 2008  12:14 -0600, Cindy Rubio Gonz�lez wrote:
> OVERWRITING ERRORS
> ==================
> fs/ext4/extents.c:2078: overwriting potential non-tentative unchecked error in "err"
> fs/ext4/extents.c:2082: overwriting potential non-tentative unchecked error in "err"
> fs/ext4/extents.c:2087: overwriting potential non-tentative unchecked error in "err"

I think these are all bugs in the analysis tool as previously mentioned.
That said, it appears that the error could be overwritten at line 2118
in the same function because the "while (i >= 0 && err == 0)" loop exits
with "err != 0" and then the check of "if (path->p_hdr->eh_entries ==
0)" is true "err" is re-used.

> OUT OF SCOPE ERRORS:
> ===================
> fs/ext4/extents.c:2864: potential non-tentative unchecked error in out of scope variable "err"
>
> UNSAVED ERRORS:
> ==============
> ext4_block_truncate_page
> fs/ext4/extents.c:2819: potential non-tentative unchecked error is not saved

Unfortunately, the "->truncate" VFS method does not allow an error to be
returned to the caller.  I suspect what we need to do here is check the
error and call ext4_warning() in this case, and mark the filesystem in
error.  I don't think calling ext4_error() is the best course of action
here, because at worst this will result in some blocks being orphaned
until e2fsck is run.  I don't think it is reasonable to force the filesystem
read-only or panic the node in such a situation, as it doesn't affect the
correctness of the data or metadata.

> mpage_da_submit_io
> fs/ext4/inode.c:1903: potential non-tentative unchecked error is not saved
> fs/ext4/inode.c:1946: potential non-tentative unchecked error is not saved
> fs/ext4/inode.c:2050: potential non-tentative unchecked error is not saved

Aneesh, could you have a look at these?  I suspect they are possibly
data-losing bugs.

> filemap_write_and_wait
> fs/ext4/inode.c:2562: potential non-tentative unchecked error is not saved

The error should just be returned to the caller.

> ext4_get_block
> {FP} fs/ext4/inode.c:3084: potential non-tentative unchecked error is not saved

The error should be returned to the caller.

> ext4_journal_test_restart
> fs/ext4/inode.c:3246: potential non-tentative unchecked error is not saved
> fs/ext4/inode.c:3456: potential non-tentative unchecked error is not saved

Actually, digging into this one, the only possible source of error is from
start_this_handle().  There are a number of places where errors could be
returned from this function, but I don't know if any of them will actually
be "new" failures in the filesystem.

  96             ret = -ENOSPC;

This should only happen as a result of a code bug, where the caller is
asking for more journal credits than can possibly fit into the journal.
That isn't to say it is impossible, however.  It might be possible to
track this via static code analysis by monitoring the maximum number
of blocks requested by all of the callers.  The minimum number of
journal transaction blocks is 256 (1/4 of the minimum journal size of
JBD2_MIN_JOURNAL_BLOCKS = 1024).

 102             new_transaction = kzalloc(sizeof(*new_transaction),
 103                                             GFP_NOFS|__GFP_NOFAIL);
 104             if (!new_transaction) {
 105                     ret = -ENOMEM;

This one can't ever be hit, because kzalloc() is called with __GFP_NOFAIL,
so the kernel should just spin here forever until memory is available.  I
suspect that "if (!new_transaction)" check was added because of previous
static analysis showing that the result of the allocation wasn't checked.

Your analysis needs to be fixed to not tag allocations with __GFP_NOFAIL
as TENTATIVE_ENOMEM.

> ext4_mb_try_best_found
> {FP} fs/ext4/mballoc.c:1843: potential non-tentative unchecked error is not saved

Does {FP} mean "false positive"?  It does appear that this is the case.
I don't necessarily like the current code construction, however.  If
the allocation status is returned in ->ac_status then the function
should just return void.

> ext4_mb_init_per_dev_proc
> fs/ext4/mballoc.c:2560: potential non-tentative unchecked error is not saved

This is a non-fatal error, and we should just make it a void function.
What it does mean is that all of the code should check if s_mb_proc != NULL
before accessing it.

> ext4_mb_destroy_per_dev_proc
> fs/ext4/mballoc.c:2653: potential non-tentative unchecked error is not saved

This shouldn't return an error either, because it is legal for s_mb_proc
to be NULL if we don't abort the mount if ext4_mb_init_per_dev_proc()
fails at mount time.  This should be a void function, and just return here.

> ext4_mb_free_metadata
> fs/ext4/mballoc.c:4697: potential non-tentative unchecked error is not saved

Ted re-wrote this code recently, so it isn't worth investigating.  It might
have been a data corruptor though...

> extend_credit_for_blkde
> fs/ext4/migrate.c:263: potential non-tentative unchecked error is not saved
> fs/ext4/migrate.c:298: potential non-tentative unchecked error is not saved
> fs/ext4/migrate.c:269: potential non-tentative unchecked error is not saved
> fs/ext4/migrate.c:309: potential non-tentative unchecked error is not saved
> fs/ext4/migrate.c:421: potential non-tentative unchecked error is not saved
> ext4_journal_restart
> fs/ext4/migrate.c:598: potential non-tentative unchecked error is not saved

I think this code is in flux, and I don't know many details of it, so I
won't review these cases.

> vfs_quota_off
> fs/ext4/super.c:1746: potential non-tentative unchecked error is not saved

Probably Jan Kara can investigate this one further.

> ext4_mb_init
> fs/ext4/super.c:2474: potential non-tentative unchecked error is not saved

In this case, if the function has an error it clears the MBALLOC option,
and the failure is harmless because that functionality is not used.
That said, in newer kernels the MBALLOC functionality is required and
this error needs to be checked and mounting failed if there is an error.

> set_blocksize
> fs/ext4/super.c:2608: potential non-tentative unchecked error is not saved

This is a false positive, in the sense that bdev_hardsect_size(bdev) was
checked a few lines earlier to be at most the filesystem blocksize, so
the only failure condition in set_blocksize() cannot be hit.  That said,
the extra bdev_hardsect_size() check in ext4_fill_super() could be removed
and we just check the return value from set_blocksize() directly.  That
removes some logic that ext4 doesn't really need to know about.

> sync_dirty_buffer
> fs/ext4/super.c:2808: potential non-tentative unchecked error is not saved

Most of the callers of ext4_commit_super() are void, so there isn't really
any place to report the error to.  Since this is an error trying to write
out the superblock, I suspect that we should return the error.  In some
places we can call ext4_error(), but not in ext4_handle_error() or a few
others (which would cause recursion).  In others like ext4_create_journal()
we can just return the error and fail the mount.

> jbd2_journal_clear_err
> fs/ext4/super.c:2868: potential non-tentative unchecked error is not saved

I don't know why ext4_clear_journal_err() is void.  The error should just be
returned to the caller and the mount failed.

> jbd2_log_wait_commit
> fs/ext4/super.c:2913: potential non-tentative unchecked error is not saved
> 
> sync_blockdev
> fs/ext4/super.c:544: potential non-tentative unchecked error is not saved

Not sure what can be done here.  The filesystem is just finished being
unmounted, so there isn't anywhere that an error can be returned, and
the filesystem can't even be marked in error.  I suppose it wouldn't
hurt to check the return value and print a warning this case like

	ext4_warning(sb, "write error at unmount, may be inconsistent\n");

> Additional:
> ==========
> ext4_mark_inode_dirty
> fs/ext4/acl.c:245: potential non-tentative unchecked error is not saved

This looks like a simple case where the error should be checked and
returned, but I don't know much about the ACL code, so it may need some
extra cleanup.

> ext4_journal_stop
> fs/ext4/acl.c:405: potential non-tentative unchecked error is not saved
> fs/ext4/acl.c:512: potential non-tentative unchecked error is not saved
> fs/ext4/extents.c:2027: potential non-tentative unchecked error is not saved
> fs/ext4/extents.c:2130: potential non-tentative unchecked error is not saved
> fs/ext4/extents.c:2863: potential non-tentative unchecked error is not saved
> :
> :

In ext4_journal_stop() it always checks and calls __ext4_std_error()
and ext4_handle_error() to handle an error.  I suspect there should be
error handling in the callers for the case where the filesystem is
mounted with errors=continue, but that is no the default behaviour of
ext4 anymore.

> ext4_forget
> ext4_ext_dirty
> ext4_mark_inode_dirty
> ext4_orphan_del
> ext4_journal_get_write_access
> ext4_orphan_add
> ext4_mark_iloc_dirty

Sorry, getting to the end of my flight here, and can't continue to
look at the rest of these error cases.  I think in most of the cases
the checker has produced valid errors.  If the {FP} cases where found
automatically, then they probably shouldn't be listed at all.  I
didn't look at any but the first one.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2008-11-22 17:39 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <adb1f7050810311819s3ea0b8f1mbb4239b108f34c4e@mail.gmail.com>
2008-11-22 17:39 ` Error Propagation bugs in ext4 Andreas Dilger
     [not found] ` <adb1f7050811041014r5be96821ie938c313aaf48334@mail.gmail.com>
2008-11-11 15:19   ` Cindy Rubio González
2008-11-22 17:39   ` Andreas Dilger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox