xfs
[Top] [All Lists]

Re: [PATCH] xfs: fix possible NULL dereference

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix possible NULL dereference
From: Geyslan GregÃrio Bem <geyslan@xxxxxxxxx>
Date: Tue, 22 Oct 2013 08:12:51 -0200
Cc: Eric Sandeen <sandeen@xxxxxxxxxxx>, Ben Myers <bpm@xxxxxxx>, Alex Elder <elder@xxxxxxxxxx>, open list <linux-kernel@xxxxxxxxxxxxxxx>, XFS FILESYSTEM <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=0kFaHMQZgz60BeMLuWYqshZTb8V4ie/OirwGs/m1mks=; b=ufUtH7kBdm1kvoyZCXHGwYkJU7ff5VYamEJWLDj2kbk8apuBenu2jReAPY0O3DBMbu 7u3oaElBYEjXiSSFcLf0nR3UH61cw/X3cjVWJxUR6923pr7R7kC2F/is9p4FHRw0+RG3 nkOyuD7Lgly6LCR3Ys57o8+mfmjYdNo0dsEAxEMVxz8vhFgtDY57W5kv5S8/VoEMzHVS Zy0zzhPk6rx4JwTvCpPH82Ems7G0XIo2I/TNuDVRkv2D0wV8SFrVn/mgf8kcfaCqG72I ENYyHfj7HTQ4GxbwU1tuHeBN5V40ZbJOv3JYEg9Vbkaw0sIWujEyVtU8+8lRLX2kVf/W d/HQ==
In-reply-to: <20131022001732.GI4446@dastard>
References: <1382380366-26540-1-git-send-email-geyslan@xxxxxxxxx> <5265956F.4010700@xxxxxxxxxxx> <20131021224459.GE16161@dastard> <5265B4D2.3000907@xxxxxxxxxxx> <20131021231849.GL10553@xxxxxxx> <20131021235601.GG4446@dastard> <5265C03B.50701@xxxxxxxxxxx> <20131022001732.GI4446@dastard>
2013/10/21 Dave Chinner <david@xxxxxxxxxxxxx>:
> On Mon, Oct 21, 2013 at 07:00:59PM -0500, Eric Sandeen wrote:
>> On 10/21/13 6:56 PM, Dave Chinner wrote:
>> > On Mon, Oct 21, 2013 at 06:18:49PM -0500, Ben Myers wrote:
>> >> Hey,
>> >>
>> >> On Mon, Oct 21, 2013 at 06:12:18PM -0500, Eric Sandeen wrote:
>> >>> On 10/21/13 5:44 PM, Dave Chinner wrote:
>> >>>> On Mon, Oct 21, 2013 at 03:58:23PM -0500, Eric Sandeen wrote:
>> >>>>> On 10/21/13 1:32 PM, Geyslan G. Bem wrote:
>> >>>>>> This patch puts a 'break' in the true branch, avoiding the 
>> >>>>>> 'icptr->ic_next'
>> >>>>>> dereferencing.
>> >>>>>
>> >>>>> Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> >>>>
>> >>>> Actually, NACK.
>> >>>
>> >>> I felt that one coming ;)
>> >>>
>> >>>>> Hm, yeah - cmn_err(CE_PANIC, " " ); used to BUG_ON, but the newer
>> >>>>> xfs_emerg() doesn't.
>> >>>>>
>> >>>>> Dave, was that intentional?
>> >>>>
>> >>>> Of course it was. ;) xfs_emerg() is only called from the debug code
>> >>>> in xlog_verify_iclog(), xlog_verify_tail_lsn and assfail().
>> >>>>
>> >>>> In the case of assfail(), it has it's own BUG() call, so it does
>> >>>> everything just fine.
>> >>>>
>> >>>> In the case of xlog_verify_iclog() when icptr is NULL, it will
>> >>>> panic immediately after the message is printed, just like the old
>> >>>> code. i.e. this patch isn't fixing anything we need fixed.
>> >>>
>> >>> A BUG() is probably warranted, then.
>> >>
>> >> I tend to agree with Eric on this point.  If we want to crash, I'd rather 
>> >> our
>> >> intent be very clear, rather than just see a null ptr deref.  ;)
>> >
>> > Sure. ASSERT() would be better and more consistent with the rest of
>> > the code. i.e:
>> >
>> >         for (i = 0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
>> >             ASSERT(icptr);
>> >
>> > <Devil's Advocate>
>> >
>> > However, I keep coming back to the fact that what we are checking is
>> > that the list is correctly circular and that and adding an
>> > ASSERT(icptr) to panic if a pointer chase finds a null pointer is
>> > kinda redundant, especially as:
>> >
>> >     - there's already 2 comments for the function indicating
>> >       that it is checking the validity of the pointers and that
>> >       they are circular....
>> >     - we have repeatedly, over many years, justified the removal
>> >       of ASSERT(ptr) from code like:
>> >
>> >             ASSERT(ptr);
>> >             foo = ptr->foo;
>> >
>> >       as it is redundant because production code will always
>> >       panic the machine in that situation via the dereference.
>> >       ASSERT() is for documenting assumptions and constraints
>> >       that are not obvious from the code context.
>> >
>> > IOWs, in this case the presence or absence of the ASSERT inside
>> > *debug-only code* doesn't add any addition value to debugging such
>> > problems, nor does it add any value in terms of documentation
>> > because it's clear from the comments in the debug code that it
>> > should not be NULL to begin with.
>> >
>> > </Devil's Advocate>
>>
>> I guess what's left as unclear is why we would prefer to panic
>> vs. handling the error, even if it's in debug code.  The caller can
>> handle errors, so blowing up here sure doesn't look intentional.
>
> If the iclog list is corrupt and not circular, then we will simply
> panic the next time it is iterated. Lots of log code iterates the
> iclog list, such as log IO completion, switching iclogs, log forces,
> etc.
>
>> Maybe the answer is it's debug code
>> and we want to drop to the debugger or generate a vmcore at that
>> point, but that's just been demonstrated as quite unclear to the
>> casual reader.  :)
>
> Yes, but to continue the Devil's Advocate argument, the purpose of
> debug code isn't to enlightent the casual reader or drive-by
> patchers - it's to make life easier for people who actually spend
> time debugging the code. And the people who need the debug code
> are expected to understand why an ASSERT is not necessary. :)
>
Dave, Eric and Ben,

This was catched by coverity (CID 102348).

Well, I got it now that was intentional and changed it in Coverity Triage.

But I must assume that it is not the standard debugging, then I
suggest you put at least a comment explaining why it does dereference
after NULL check.

Cheers.

> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>