linux-ext4 - Re: [PATCH 1/3] e2fsck: Clarify overflow link count error message

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:   Thu, 6 Feb 2020 10:59:19 +0100
From:   Jan Kara <jack@...e.cz>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/3] e2fsck: Clarify overflow link count error message

On Wed 05-02-20 10:38:04, Andreas Dilger wrote:
> On Feb 5, 2020, at 3:01 AM, Jan Kara <jack@...e.cz> wrote:
> > 
> > When directory link count is set to overflow value (1) but during pass 4
> > we find out the exact link count would fit, we either silently fix this
> > (which is not great because e2fsck then reports the fs was modified but
> > output doesn't indicate why in any way), or we report that link count is
> > wrong and ask whether we should fix it (in case -n option was
> > specified). The second case is even more misleading because it suggests
> > non-trivial fs corruption which then gets silently fixed on the next
> > run. Similarly to how we fix up other non-problems, just create a new
> > error message for the case directory link count is not overflown anymore
> > and always report it to clarify what is going on.
> > 
> > 
> > diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> > index c7c0ba986006..cde369d03034 100644
> > --- a/e2fsck/problem.c
> > +++ b/e2fsck/problem.c
> > @@ -2035,6 +2035,11 @@ static struct e2fsck_problem problem_table[] = {
> > 	  N_("@d exceeds max links, but no DIR_NLINK feature in @S.\n"),
> > 	  PROMPT_FIX, 0, 0, 0, 0 },
> > 
> > +	/* Directory ref count set to overflow but it doesn't have to be */
> 
> > +	{ PR_4_DIR_OVERFLOW_REF_COUNT,
> > +	  N_("@d @i %i ref count set to overflow value %Il but could be exact value %N.  "),
> 
> IMHO, you don't need to print "value %Il" since that will always be "1"?
> That would shorten the message to fit on a single line.

Yeah, will change.

> Also, lease keep the comment and the actual error message identical.
> Otherwise, it is harder to find the PR_* number and the related code in
> e2fsck when trying to debug a problem.  So the comment should be:
> 
> 	/* Directory inode ref count set to overflow but could be exact value */

Sure, thanks for catching this.

> To be honest, I don't see the benefit of the @d, @i, etc. abbreviations
> in the messages anymore.  The few bytes of space savings is IMHO outweighed
> by the added complexity in understanding and finding the messages in the
> code, and added complexity in e2fsck itself when printing the messages.

I tend to agree but I was never bothered enough to try to change that.

									Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR