[PATCH] xfs: fix possible NULL dereference

[PATCH] xfs: fix possible NULL dereference

[Geyslan G. Bem]

This patch puts a 'break' in the true branch, avoiding the 'icptr->ic_next'
dereferencing.

Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
 fs/xfs/xfs_log.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a2dea108..8cdeb7e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3703,8 +3703,10 @@ xlog_verify_iclog(
 	spin_lock(&log->l_icloglock);
 	icptr = log->l_iclog;
 	for (i=0; i < log->l_iclog_bufs; i++) {
-		if (icptr == NULL)
+		if (!icptr) {
 			xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
+			break;
+		}
 		icptr = icptr->ic_next;
 	}
 	if (icptr != log->l_iclog)
-- 
1.8.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] xfs: fix possible NULL dereference

[Geyslan G. Bem]

This patch puts a 'break' in the true branch, avoiding the 'icptr->ic_next'
dereferencing.

Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
 fs/xfs/xfs_log.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a2dea108..8cdeb7e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3703,8 +3703,10 @@ xlog_verify_iclog(
 	spin_lock(&log->l_icloglock);
 	icptr = log->l_iclog;
 	for (i=0; i < log->l_iclog_bufs; i++) {
-		if (icptr == NULL)
+		if (!icptr) {
 			xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
+			break;
+		}
 		icptr = icptr->ic_next;
 	}
 	if (icptr != log->l_iclog)
-- 
1.8.4

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

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@redhat.com>

Hm, yeah - cmn_err(CE_PANIC, " " ); used to BUG_ON, but the newer
xfs_emerg() doesn't.

Dave, was that intentional?

I wonder if there are more spots after xfs_emerg()'s which aren't
defensive, because the code used to just panic there.

-Eric


> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> ---
>  fs/xfs/xfs_log.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a2dea108..8cdeb7e 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3703,8 +3703,10 @@ xlog_verify_iclog(
>  	spin_lock(&log->l_icloglock);
>  	icptr = log->l_iclog;
>  	for (i=0; i < log->l_iclog_bufs; i++) {
> -		if (icptr == NULL)
> +		if (!icptr) {
>  			xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> +			break;
> +		}
>  		icptr = icptr->ic_next;
>  	}
>  	if (icptr != log->l_iclog)
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

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@redhat.com>

Actually, NACK.

> 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. 

> I wonder if there are more spots after xfs_emerg()'s which aren't
> defensive, because the code used to just panic there.

As for the rest of the calls in xlog_verify_iclog, they are checking
things that aren't immediately fatal, but indication that iclog
corruption has already occurred. It's debug code, so we could add
"panic immediately" code, but personally I'd prefer to see the error
message being printed and then have it continue like a production
system would so that we can see the types of crashes normal kernels
will see as a result of iclog memory corruption....

As for xlog_verify_tail_lsn(), that's an important informational
message indicating we might be leaking log space. It's not
immediately fatal, but if we see it and then have a log space
hang...

So, really, none of the callers really need xfs_emerg to panic like
CE_PANIC used to. The one case where it might be useful (i.e this
patch) we panic immediately anyway....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

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@redhat.com>

Actually, NACK.

> 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. 

> I wonder if there are more spots after xfs_emerg()'s which aren't
> defensive, because the code used to just panic there.

As for the rest of the calls in xlog_verify_iclog, they are checking
things that aren't immediately fatal, but indication that iclog
corruption has already occurred. It's debug code, so we could add
"panic immediately" code, but personally I'd prefer to see the error
message being printed and then have it continue like a production
system would so that we can see the types of crashes normal kernels
will see as a result of iclog memory corruption....

As for xlog_verify_tail_lsn(), that's an important informational
message indicating we might be leaking log space. It's not
immediately fatal, but if we see it and then have a log space
hang...

So, really, none of the callers really need xfs_emerg to panic like
CE_PANIC used to. The one case where it might be useful (i.e this
patch) we panic immediately anyway....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

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@redhat.com>
> 
> 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.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

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@redhat.com>
> 
> 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.

-Eric

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

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@redhat.com>
> > 
> > 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.  ;)

Regards,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

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@redhat.com>
> > 
> > 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.  ;)

Regards,
	Ben

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

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@redhat.com>
> > > 
> > > 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>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

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@redhat.com>
> > > 
> > > 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>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

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@redhat.com>
>>>>
>>>> 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.

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.  :)

-Eric

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

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@redhat.com>
>>>>
>>>> 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.

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.  :)

-Eric

> Cheers,
> 
> Dave.
> 

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

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@redhat.com>
> >>>>
> >>>> 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. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

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@redhat.com>
> >>>>
> >>>> 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. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/21 Dave Chinner <david@fromorbit.com>:
> 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@redhat.com>
>> >>>>
>> >>>> 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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/21 Dave Chinner <david@fromorbit.com>:
> 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@redhat.com>
>> >>>>
>> >>>> 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@fromorbit.com

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> > 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:
> >
> > 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).

You should have put that in the patch description.

Now I understand why there's been a sudden surge of irrelevant one
line changes from random people that have never touched XFS before.

<sigh>

Ok, lets churn the code just to shut the stupid checker up. This
doesn't fix a bug, it doesn't change behaviour, it just makes
coverity happy. Convert it to the for loop plus ASSERT I mentioned
in a previous message.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> > 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:
> >
> > 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).

You should have put that in the patch description.

Now I understand why there's been a sudden surge of irrelevant one
line changes from random people that have never touched XFS before.

<sigh>

Ok, lets churn the code just to shut the stupid checker up. This
doesn't fix a bug, it doesn't change behaviour, it just makes
coverity happy. Convert it to the for loop plus ASSERT I mentioned
in a previous message.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

On 10/22/13 3:39 PM, Dave Chinner wrote:
> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
>>> 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:
>>>
>>> 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).
> 
> You should have put that in the patch description.
> 
> Now I understand why there's been a sudden surge of irrelevant one
> line changes from random people that have never touched XFS before.
> 
> <sigh>
> 
> Ok, lets churn the code just to shut the stupid checker up. This
> doesn't fix a bug, it doesn't change behaviour, it just makes
> coverity happy. Convert it to the for loop plus ASSERT I mentioned
> in a previous message.

You know, I respectfully disagree, but we might just have to agree
to disagree.  The code, as it stands, tests for a null ptr
and then dereferences it.  That's always going to raise some
eyebrows, coverity or not, debug code or not, drive by or not.


So even for future developers, making the code more self-
documenting about this behavior would be a plus, whether it's by
comment, by explicit ASSERT(), or whatever.  (I don't think
that xfs_emerg() has quite enough context to make it obvious.)

(We don't have to change code to shut up coverity; we can flag
it in the database and nobody else will see it.)

-Eric

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

On 10/22/13 3:39 PM, Dave Chinner wrote:
> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
>>> 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:
>>>
>>> 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).
> 
> You should have put that in the patch description.
> 
> Now I understand why there's been a sudden surge of irrelevant one
> line changes from random people that have never touched XFS before.
> 
> <sigh>
> 
> Ok, lets churn the code just to shut the stupid checker up. This
> doesn't fix a bug, it doesn't change behaviour, it just makes
> coverity happy. Convert it to the for loop plus ASSERT I mentioned
> in a previous message.

You know, I respectfully disagree, but we might just have to agree
to disagree.  The code, as it stands, tests for a null ptr
and then dereferences it.  That's always going to raise some
eyebrows, coverity or not, debug code or not, drive by or not.


So even for future developers, making the code more self-
documenting about this behavior would be a plus, whether it's by
comment, by explicit ASSERT(), or whatever.  (I don't think
that xfs_emerg() has quite enough context to make it obvious.)

(We don't have to change code to shut up coverity; we can flag
it in the database and nobody else will see it.)

-Eric

> Cheers,
> 
> Dave.
> 

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
> On 10/22/13 3:39 PM, Dave Chinner wrote:
> > On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> >> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> >>> 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:
> >>>
> >>> 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).
> > 
> > You should have put that in the patch description.
> > 
> > Now I understand why there's been a sudden surge of irrelevant one
> > line changes from random people that have never touched XFS before.
> > 
> > <sigh>
> > 
> > Ok, lets churn the code just to shut the stupid checker up. This
> > doesn't fix a bug, it doesn't change behaviour, it just makes
> > coverity happy. Convert it to the for loop plus ASSERT I mentioned
> > in a previous message.
> 
> You know, I respectfully disagree, but we might just have to agree
> to disagree.  The code, as it stands, tests for a null ptr
> and then dereferences it.  That's always going to raise some
> eyebrows, coverity or not, debug code or not, drive by or not.

> So even for future developers, making the code more self-
> documenting about this behavior would be a plus, whether it's by
> comment, by explicit ASSERT(), or whatever.  (I don't think
> that xfs_emerg() has quite enough context to make it obvious.)

Sure, but if weren't for the fact that Coverity warned about it,
nobody other that us people who work on the XFS code day in, day out
would have even cared about it.

That's kind of my point - again, as the Devil's Advocate - that
coverity is encouraging drive-by "fixes" by people who don't
actually understand any of the context, history and/or culture
surrounding the code being modified.

I have no problems with real bugs being fixed, but if we are
modifying code for no gain other than closing "coverity doesn't like
it" bugs, then we *should* be questioning whether the change is
really necessary.

Asking the question may not change the outcome, but we need to ask
and answer the question regardless.

> (We don't have to change code to shut up coverity; we can flag
> it in the database and nobody else will see it.)

Only if you are the first to see it and make an executive decision
that it's not necessary to fix.... :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
> On 10/22/13 3:39 PM, Dave Chinner wrote:
> > On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> >> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> >>> 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:
> >>>
> >>> 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).
> > 
> > You should have put that in the patch description.
> > 
> > Now I understand why there's been a sudden surge of irrelevant one
> > line changes from random people that have never touched XFS before.
> > 
> > <sigh>
> > 
> > Ok, lets churn the code just to shut the stupid checker up. This
> > doesn't fix a bug, it doesn't change behaviour, it just makes
> > coverity happy. Convert it to the for loop plus ASSERT I mentioned
> > in a previous message.
> 
> You know, I respectfully disagree, but we might just have to agree
> to disagree.  The code, as it stands, tests for a null ptr
> and then dereferences it.  That's always going to raise some
> eyebrows, coverity or not, debug code or not, drive by or not.

> So even for future developers, making the code more self-
> documenting about this behavior would be a plus, whether it's by
> comment, by explicit ASSERT(), or whatever.  (I don't think
> that xfs_emerg() has quite enough context to make it obvious.)

Sure, but if weren't for the fact that Coverity warned about it,
nobody other that us people who work on the XFS code day in, day out
would have even cared about it.

That's kind of my point - again, as the Devil's Advocate - that
coverity is encouraging drive-by "fixes" by people who don't
actually understand any of the context, history and/or culture
surrounding the code being modified.

I have no problems with real bugs being fixed, but if we are
modifying code for no gain other than closing "coverity doesn't like
it" bugs, then we *should* be questioning whether the change is
really necessary.

Asking the question may not change the outcome, but we need to ask
and answer the question regardless.

> (We don't have to change code to shut up coverity; we can flag
> it in the database and nobody else will see it.)

Only if you are the first to see it and make an executive decision
that it's not necessary to fix.... :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

On 10/22/13 4:03 PM, Dave Chinner wrote:
> On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
>> On 10/22/13 3:39 PM, Dave Chinner wrote:
>>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
>>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
>>>>> 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:
>>>>>
>>>>> 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).
>>>
>>> You should have put that in the patch description.
>>>
>>> Now I understand why there's been a sudden surge of irrelevant one
>>> line changes from random people that have never touched XFS before.
>>>
>>> <sigh>
>>>
>>> Ok, lets churn the code just to shut the stupid checker up. This
>>> doesn't fix a bug, it doesn't change behaviour, it just makes
>>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
>>> in a previous message.
>>
>> You know, I respectfully disagree, but we might just have to agree
>> to disagree.  The code, as it stands, tests for a null ptr
>> and then dereferences it.  That's always going to raise some
>> eyebrows, coverity or not, debug code or not, drive by or not.
> 
>> So even for future developers, making the code more self-
>> documenting about this behavior would be a plus, whether it's by
>> comment, by explicit ASSERT(), or whatever.  (I don't think
>> that xfs_emerg() has quite enough context to make it obvious.)
> 
> Sure, but if weren't for the fact that Coverity warned about it,
> nobody other that us people who work on the XFS code day in, day out
> would have even cared about it.
> 
> That's kind of my point - again, as the Devil's Advocate - that
> coverity is encouraging drive-by "fixes" by people who don't
> actually understand any of the context, history and/or culture
> surrounding the code being modified.

They shouldn't have to, the code (or comments therein) should
make it obvious.  ;)  (in a perfect world...)

> I have no problems with real bugs being fixed, but if we are
> modifying code for no gain other than closing "coverity doesn't like
> it" bugs, then we *should* be questioning whether the change is
> really necessary.

But let's give Geyslan the benefit of the doubt, and realize that
Coverity does find real things, and even if it originated w/ a
Coverity CID, when one sees:

	if (!a)
		printk("a thing\n")

	a = a->b = . . . 

it looks suspicious to pretty much anyone.  I don't think Geyslan
sent it to shut Coverity up, he sent it because it looked like
a bug worth fixing (after Coverity spotted it).

Let's not be too hard on him for trying; I appreciate it more
than spelling fixes and whitespace cleanups.  ;)

I agree that some Coverity CIDs are false, and we shouldn't
mangle code just to make it happy, but I just don't think that's
what's going on here.  Let's imagine Geyslan saw 10 other CIDs
and elected not to send changes, because they didn't look like
they needed fixing, but this one looked like a bona fide bug.

> Asking the question may not change the outcome, but we need to ask
> and answer the question regardless.
 
>> (We don't have to change code to shut up coverity; we can flag
>> it in the database and nobody else will see it.)
> 
> Only if you are the first to see it and make an executive decision
> that it's not necessary to fix.... :/

Or, you find it, send a patch that seems reasonable, get massive
pushback, (hopefully) flag it, and resolve never come back to xfs
again.  ;)

-Eric
 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

On 10/22/13 4:03 PM, Dave Chinner wrote:
> On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
>> On 10/22/13 3:39 PM, Dave Chinner wrote:
>>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
>>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
>>>>> 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:
>>>>>
>>>>> 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).
>>>
>>> You should have put that in the patch description.
>>>
>>> Now I understand why there's been a sudden surge of irrelevant one
>>> line changes from random people that have never touched XFS before.
>>>
>>> <sigh>
>>>
>>> Ok, lets churn the code just to shut the stupid checker up. This
>>> doesn't fix a bug, it doesn't change behaviour, it just makes
>>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
>>> in a previous message.
>>
>> You know, I respectfully disagree, but we might just have to agree
>> to disagree.  The code, as it stands, tests for a null ptr
>> and then dereferences it.  That's always going to raise some
>> eyebrows, coverity or not, debug code or not, drive by or not.
> 
>> So even for future developers, making the code more self-
>> documenting about this behavior would be a plus, whether it's by
>> comment, by explicit ASSERT(), or whatever.  (I don't think
>> that xfs_emerg() has quite enough context to make it obvious.)
> 
> Sure, but if weren't for the fact that Coverity warned about it,
> nobody other that us people who work on the XFS code day in, day out
> would have even cared about it.
> 
> That's kind of my point - again, as the Devil's Advocate - that
> coverity is encouraging drive-by "fixes" by people who don't
> actually understand any of the context, history and/or culture
> surrounding the code being modified.

They shouldn't have to, the code (or comments therein) should
make it obvious.  ;)  (in a perfect world...)

> I have no problems with real bugs being fixed, but if we are
> modifying code for no gain other than closing "coverity doesn't like
> it" bugs, then we *should* be questioning whether the change is
> really necessary.

But let's give Geyslan the benefit of the doubt, and realize that
Coverity does find real things, and even if it originated w/ a
Coverity CID, when one sees:

	if (!a)
		printk("a thing\n")

	a = a->b = . . . 

it looks suspicious to pretty much anyone.  I don't think Geyslan
sent it to shut Coverity up, he sent it because it looked like
a bug worth fixing (after Coverity spotted it).

Let's not be too hard on him for trying; I appreciate it more
than spelling fixes and whitespace cleanups.  ;)

I agree that some Coverity CIDs are false, and we shouldn't
mangle code just to make it happy, but I just don't think that's
what's going on here.  Let's imagine Geyslan saw 10 other CIDs
and elected not to send changes, because they didn't look like
they needed fixing, but this one looked like a bona fide bug.

> Asking the question may not change the outcome, but we need to ask
> and answer the question regardless.
 
>> (We don't have to change code to shut up coverity; we can flag
>> it in the database and nobody else will see it.)
> 
> Only if you are the first to see it and make an executive decision
> that it's not necessary to fix.... :/

Or, you find it, send a patch that seems reasonable, get massive
pushback, (hopefully) flag it, and resolve never come back to xfs
again.  ;)

-Eric
 
> Cheers,
> 
> Dave.
> 

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

On Tue, Oct 22, 2013 at 04:19:44PM -0500, Eric Sandeen wrote:
> On 10/22/13 4:03 PM, Dave Chinner wrote:
> > On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
> >> On 10/22/13 3:39 PM, Dave Chinner wrote:
> >>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> >>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> >>>>> 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:
> >>>>>
> >>>>> 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).
> >>>
> >>> You should have put that in the patch description.
> >>>
> >>> Now I understand why there's been a sudden surge of irrelevant one
> >>> line changes from random people that have never touched XFS before.
> >>>
> >>> <sigh>
> >>>
> >>> Ok, lets churn the code just to shut the stupid checker up. This
> >>> doesn't fix a bug, it doesn't change behaviour, it just makes
> >>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
> >>> in a previous message.
> >>
> >> You know, I respectfully disagree, but we might just have to agree
> >> to disagree.  The code, as it stands, tests for a null ptr
> >> and then dereferences it.  That's always going to raise some
> >> eyebrows, coverity or not, debug code or not, drive by or not.
> > 
> >> So even for future developers, making the code more self-
> >> documenting about this behavior would be a plus, whether it's by
> >> comment, by explicit ASSERT(), or whatever.  (I don't think
> >> that xfs_emerg() has quite enough context to make it obvious.)
> > 
> > Sure, but if weren't for the fact that Coverity warned about it,
> > nobody other that us people who work on the XFS code day in, day out
> > would have even cared about it.
> > 
> > That's kind of my point - again, as the Devil's Advocate - that
> > coverity is encouraging drive-by "fixes" by people who don't
> > actually understand any of the context, history and/or culture
> > surrounding the code being modified.
> 
> They shouldn't have to, the code (or comments therein) should
> make it obvious.  ;)  (in a perfect world...)

Obvious to whom, exactly?

That's the point I'm trying to make - "#ifdef DEBUG", two
comments indicating that it's validating the list and printing a
message just before it goes boom. That's pretty obvious code to
anyone who is used to tracking down corrupted list problems...

> > I have no problems with real bugs being fixed, but if we are
> > modifying code for no gain other than closing "coverity doesn't like
> > it" bugs, then we *should* be questioning whether the change is
> > really necessary.
> 
> But let's give Geyslan the benefit of the doubt, and realize that
> Coverity does find real things, and even if it originated w/ a
> Coverity CID, when one sees:
> 
> 	if (!a)
> 		printk("a thing\n")
> 
> 	a = a->b = . . . 
> 
> it looks suspicious to pretty much anyone.  I don't think Geyslan
> sent it to shut Coverity up, he sent it because it looked like
> a bug worth fixing (after Coverity spotted it).
> 
> Let's not be too hard on him for trying; I appreciate it more
> than spelling fixes and whitespace cleanups.  ;)

True, point taken. 

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Chinner]

On Tue, Oct 22, 2013 at 04:19:44PM -0500, Eric Sandeen wrote:
> On 10/22/13 4:03 PM, Dave Chinner wrote:
> > On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
> >> On 10/22/13 3:39 PM, Dave Chinner wrote:
> >>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> >>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> >>>>> 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:
> >>>>>
> >>>>> 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).
> >>>
> >>> You should have put that in the patch description.
> >>>
> >>> Now I understand why there's been a sudden surge of irrelevant one
> >>> line changes from random people that have never touched XFS before.
> >>>
> >>> <sigh>
> >>>
> >>> Ok, lets churn the code just to shut the stupid checker up. This
> >>> doesn't fix a bug, it doesn't change behaviour, it just makes
> >>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
> >>> in a previous message.
> >>
> >> You know, I respectfully disagree, but we might just have to agree
> >> to disagree.  The code, as it stands, tests for a null ptr
> >> and then dereferences it.  That's always going to raise some
> >> eyebrows, coverity or not, debug code or not, drive by or not.
> > 
> >> So even for future developers, making the code more self-
> >> documenting about this behavior would be a plus, whether it's by
> >> comment, by explicit ASSERT(), or whatever.  (I don't think
> >> that xfs_emerg() has quite enough context to make it obvious.)
> > 
> > Sure, but if weren't for the fact that Coverity warned about it,
> > nobody other that us people who work on the XFS code day in, day out
> > would have even cared about it.
> > 
> > That's kind of my point - again, as the Devil's Advocate - that
> > coverity is encouraging drive-by "fixes" by people who don't
> > actually understand any of the context, history and/or culture
> > surrounding the code being modified.
> 
> They shouldn't have to, the code (or comments therein) should
> make it obvious.  ;)  (in a perfect world...)

Obvious to whom, exactly?

That's the point I'm trying to make - "#ifdef DEBUG", two
comments indicating that it's validating the list and printing a
message just before it goes boom. That's pretty obvious code to
anyone who is used to tracking down corrupted list problems...

> > I have no problems with real bugs being fixed, but if we are
> > modifying code for no gain other than closing "coverity doesn't like
> > it" bugs, then we *should* be questioning whether the change is
> > really necessary.
> 
> But let's give Geyslan the benefit of the doubt, and realize that
> Coverity does find real things, and even if it originated w/ a
> Coverity CID, when one sees:
> 
> 	if (!a)
> 		printk("a thing\n")
> 
> 	a = a->b = . . . 
> 
> it looks suspicious to pretty much anyone.  I don't think Geyslan
> sent it to shut Coverity up, he sent it because it looked like
> a bug worth fixing (after Coverity spotted it).
> 
> Let's not be too hard on him for trying; I appreciate it more
> than spelling fixes and whitespace cleanups.  ;)

True, point taken. 

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

Hey Gents,

On Wed, Oct 23, 2013 at 09:02:54AM +1100, Dave Chinner wrote:
> On Tue, Oct 22, 2013 at 04:19:44PM -0500, Eric Sandeen wrote:
> > On 10/22/13 4:03 PM, Dave Chinner wrote:
> > > On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
> > >> On 10/22/13 3:39 PM, Dave Chinner wrote:
> > >>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> > >>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> > >>>>> 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:
> > >>>>>
> > >>>>> 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).
> > >>>
> > >>> You should have put that in the patch description.
> > >>>
> > >>> Now I understand why there's been a sudden surge of irrelevant one
> > >>> line changes from random people that have never touched XFS before.
> > >>>
> > >>> <sigh>
> > >>>
> > >>> Ok, lets churn the code just to shut the stupid checker up. This
> > >>> doesn't fix a bug, it doesn't change behaviour, it just makes
> > >>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
> > >>> in a previous message.
> > >>
> > >> You know, I respectfully disagree, but we might just have to agree
> > >> to disagree.  The code, as it stands, tests for a null ptr
> > >> and then dereferences it.  That's always going to raise some
> > >> eyebrows, coverity or not, debug code or not, drive by or not.
> > > 
> > >> So even for future developers, making the code more self-
> > >> documenting about this behavior would be a plus, whether it's by
> > >> comment, by explicit ASSERT(), or whatever.  (I don't think
> > >> that xfs_emerg() has quite enough context to make it obvious.)
> > > 
> > > Sure, but if weren't for the fact that Coverity warned about it,
> > > nobody other that us people who work on the XFS code day in, day out
> > > would have even cared about it.
> > > 
> > > That's kind of my point - again, as the Devil's Advocate - that
> > > coverity is encouraging drive-by "fixes" by people who don't
> > > actually understand any of the context, history and/or culture
> > > surrounding the code being modified.
> > 
> > They shouldn't have to, the code (or comments therein) should
> > make it obvious.  ;)  (in a perfect world...)
> 
> Obvious to whom, exactly?
> 
> That's the point I'm trying to make - "#ifdef DEBUG", two
> comments indicating that it's validating the list and printing a
> message just before it goes boom. That's pretty obvious code to
> anyone who is used to tracking down corrupted list problems...
> 
> > > I have no problems with real bugs being fixed, but if we are
> > > modifying code for no gain other than closing "coverity doesn't like
> > > it" bugs, then we *should* be questioning whether the change is
> > > really necessary.
> > 
> > But let's give Geyslan the benefit of the doubt, and realize that
> > Coverity does find real things, and even if it originated w/ a
> > Coverity CID, when one sees:
> > 
> > 	if (!a)
> > 		printk("a thing\n")
> > 
> > 	a = a->b = . . . 
> > 
> > it looks suspicious to pretty much anyone.  I don't think Geyslan
> > sent it to shut Coverity up, he sent it because it looked like
> > a bug worth fixing (after Coverity spotted it).
> > 
> > Let's not be too hard on him for trying; I appreciate it more
> > than spelling fixes and whitespace cleanups.  ;)
> 
> True, point taken. 

So, uh, lets go with the ASSERT approach then?  It seems to be a reasonable
middle ground.  ;)

Regards,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

Hey Gents,

On Wed, Oct 23, 2013 at 09:02:54AM +1100, Dave Chinner wrote:
> On Tue, Oct 22, 2013 at 04:19:44PM -0500, Eric Sandeen wrote:
> > On 10/22/13 4:03 PM, Dave Chinner wrote:
> > > On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
> > >> On 10/22/13 3:39 PM, Dave Chinner wrote:
> > >>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
> > >>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
> > >>>>> 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:
> > >>>>>
> > >>>>> 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).
> > >>>
> > >>> You should have put that in the patch description.
> > >>>
> > >>> Now I understand why there's been a sudden surge of irrelevant one
> > >>> line changes from random people that have never touched XFS before.
> > >>>
> > >>> <sigh>
> > >>>
> > >>> Ok, lets churn the code just to shut the stupid checker up. This
> > >>> doesn't fix a bug, it doesn't change behaviour, it just makes
> > >>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
> > >>> in a previous message.
> > >>
> > >> You know, I respectfully disagree, but we might just have to agree
> > >> to disagree.  The code, as it stands, tests for a null ptr
> > >> and then dereferences it.  That's always going to raise some
> > >> eyebrows, coverity or not, debug code or not, drive by or not.
> > > 
> > >> So even for future developers, making the code more self-
> > >> documenting about this behavior would be a plus, whether it's by
> > >> comment, by explicit ASSERT(), or whatever.  (I don't think
> > >> that xfs_emerg() has quite enough context to make it obvious.)
> > > 
> > > Sure, but if weren't for the fact that Coverity warned about it,
> > > nobody other that us people who work on the XFS code day in, day out
> > > would have even cared about it.
> > > 
> > > That's kind of my point - again, as the Devil's Advocate - that
> > > coverity is encouraging drive-by "fixes" by people who don't
> > > actually understand any of the context, history and/or culture
> > > surrounding the code being modified.
> > 
> > They shouldn't have to, the code (or comments therein) should
> > make it obvious.  ;)  (in a perfect world...)
> 
> Obvious to whom, exactly?
> 
> That's the point I'm trying to make - "#ifdef DEBUG", two
> comments indicating that it's validating the list and printing a
> message just before it goes boom. That's pretty obvious code to
> anyone who is used to tracking down corrupted list problems...
> 
> > > I have no problems with real bugs being fixed, but if we are
> > > modifying code for no gain other than closing "coverity doesn't like
> > > it" bugs, then we *should* be questioning whether the change is
> > > really necessary.
> > 
> > But let's give Geyslan the benefit of the doubt, and realize that
> > Coverity does find real things, and even if it originated w/ a
> > Coverity CID, when one sees:
> > 
> > 	if (!a)
> > 		printk("a thing\n")
> > 
> > 	a = a->b = . . . 
> > 
> > it looks suspicious to pretty much anyone.  I don't think Geyslan
> > sent it to shut Coverity up, he sent it because it looked like
> > a bug worth fixing (after Coverity spotted it).
> > 
> > Let's not be too hard on him for trying; I appreciate it more
> > than spelling fixes and whitespace cleanups.  ;)
> 
> True, point taken. 

So, uh, lets go with the ASSERT approach then?  It seems to be a reasonable
middle ground.  ;)

Regards,
	Ben

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Jones]

On Wed, Oct 23, 2013 at 09:02:54AM +1100, Dave Chinner wrote:

 > > it looks suspicious to pretty much anyone.  I don't think Geyslan
 > > sent it to shut Coverity up, he sent it because it looked like
 > > a bug worth fixing (after Coverity spotted it).
 > > 
 > > Let's not be too hard on him for trying; I appreciate it more
 > > than spelling fixes and whitespace cleanups.  ;)
 > 
 > True, point taken. 

So another reason you're seeing an uptick in coverity reports lately
is that back in June they gave me admin rights for the project at scan.coverity.com
so I've been doing daily builds since then. (Previously they only did one per point release).

The Coverity guys did a write-up on this thread at http://security.coverity.com/blog/2013/Oct/deliberate-null-pointer-dereferences-in-the-linux-kernel.html 
The point about modelling is the pertinent part.  I'm still trying to get my
head around a lot of how that stuff works, but that's the sort of thing
that I have rights to do on their site too.

If you or anyone else wants access to their bugs, I can approve that
easily enough.  I've been going through and trying to filter out as many of
the intentional[*] issues as possible, and do things like sorting into components
so that you're able to look at just XFS bugs for eg.

I know Eric has been looking at their bugs when he has had time, but if there's
something I can do to make things easier for you guys, let me know.
(I could email you new issue reports as they come in for eg)

To end on a high note, XFS is actually one of the better subsystems from the
POV of number of issues they've found. Only 38 'New' issues right now, which
given the complexity in XFS, is pretty darn good, and I bet a bunch of those
are actually non-issues too.  The painful part is going through and sorting
through the non-issues to get to the real meaty bugs, which is what I've slowly
been doing over the last couple months. (Down from 5900 or so, to 5305,
thanks to help from others)

	Dave

[*] From what I've seen so far, a lot of issues it finds are the checker
getting tricked by idioms we use in the kernel rather than actual "false positives"
(in terms of "this is a bug in the checker"). As the url above points out,
sometimes we can help the checker out through modelling, but some of the code
I've seen it get tripped up is hard enough for a human to parse, so I don't 
really blame the checker for getting confused ;)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Dave Jones]

On Wed, Oct 23, 2013 at 09:02:54AM +1100, Dave Chinner wrote:

 > > it looks suspicious to pretty much anyone.  I don't think Geyslan
 > > sent it to shut Coverity up, he sent it because it looked like
 > > a bug worth fixing (after Coverity spotted it).
 > > 
 > > Let's not be too hard on him for trying; I appreciate it more
 > > than spelling fixes and whitespace cleanups.  ;)
 > 
 > True, point taken. 

So another reason you're seeing an uptick in coverity reports lately
is that back in June they gave me admin rights for the project at scan.coverity.com
so I've been doing daily builds since then. (Previously they only did one per point release).

The Coverity guys did a write-up on this thread at http://security.coverity.com/blog/2013/Oct/deliberate-null-pointer-dereferences-in-the-linux-kernel.html 
The point about modelling is the pertinent part.  I'm still trying to get my
head around a lot of how that stuff works, but that's the sort of thing
that I have rights to do on their site too.

If you or anyone else wants access to their bugs, I can approve that
easily enough.  I've been going through and trying to filter out as many of
the intentional[*] issues as possible, and do things like sorting into components
so that you're able to look at just XFS bugs for eg.

I know Eric has been looking at their bugs when he has had time, but if there's
something I can do to make things easier for you guys, let me know.
(I could email you new issue reports as they come in for eg)

To end on a high note, XFS is actually one of the better subsystems from the
POV of number of issues they've found. Only 38 'New' issues right now, which
given the complexity in XFS, is pretty darn good, and I bet a bunch of those
are actually non-issues too.  The painful part is going through and sorting
through the non-issues to get to the real meaty bugs, which is what I've slowly
been doing over the last couple months. (Down from 5900 or so, to 5305,
thanks to help from others)

	Dave

[*] From what I've seen so far, a lot of issues it finds are the checker
getting tricked by idioms we use in the kernel rather than actual "false positives"
(in terms of "this is a bug in the checker"). As the url above points out,
sometimes we can help the checker out through modelling, but some of the code
I've seen it get tripped up is hard enough for a human to parse, so I don't 
really blame the checker for getting confused ;)

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/22 Eric Sandeen <sandeen@sandeen.net>:
> On 10/22/13 4:03 PM, Dave Chinner wrote:
>> On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
>>> On 10/22/13 3:39 PM, Dave Chinner wrote:
>>>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
>>>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
>>>>>> 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:
>>>>>>
>>>>>> 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).
>>>>
>>>> You should have put that in the patch description.
>>>>
>>>> Now I understand why there's been a sudden surge of irrelevant one
>>>> line changes from random people that have never touched XFS before.
>>>>
>>>> <sigh>
>>>>
>>>> Ok, lets churn the code just to shut the stupid checker up. This
>>>> doesn't fix a bug, it doesn't change behaviour, it just makes
>>>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
>>>> in a previous message.
>>>
>>> You know, I respectfully disagree, but we might just have to agree
>>> to disagree.  The code, as it stands, tests for a null ptr
>>> and then dereferences it.  That's always going to raise some
>>> eyebrows, coverity or not, debug code or not, drive by or not.
>>
>>> So even for future developers, making the code more self-
>>> documenting about this behavior would be a plus, whether it's by
>>> comment, by explicit ASSERT(), or whatever.  (I don't think
>>> that xfs_emerg() has quite enough context to make it obvious.)
>>
>> Sure, but if weren't for the fact that Coverity warned about it,
>> nobody other that us people who work on the XFS code day in, day out
>> would have even cared about it.
>>
>> That's kind of my point - again, as the Devil's Advocate - that
>> coverity is encouraging drive-by "fixes" by people who don't
>> actually understand any of the context, history and/or culture
>> surrounding the code being modified.
>
Dave, If Coverity had not caught this, I could have never sent a patch
to xfs in my entire life.
So, I need not to know the xfs culture, code or context to identify a
flagrant, intentional or not, code that seems a bug.
Open source world works this way, all can help, but only a few decide
to do it. And there many ways to do it; static analysis is only one.

> They shouldn't have to, the code (or comments therein) should
> make it obvious.  ;)  (in a perfect world...)
>
>> I have no problems with real bugs being fixed, but if we are
>> modifying code for no gain other than closing "coverity doesn't like
>> it" bugs, then we *should* be questioning whether the change is
>> really necessary.
>
> But let's give Geyslan the benefit of the doubt, and realize that
> Coverity does find real things, and even if it originated w/ a
> Coverity CID, when one sees:
>
>         if (!a)
>                 printk("a thing\n")
>
>         a = a->b = . . .
>
> it looks suspicious to pretty much anyone.  I don't think Geyslan
> sent it to shut Coverity up, he sent it because it looked like
> a bug worth fixing (after Coverity spotted it).
>
Eric, you're right. In this particular case, I didn't send to shut Coverity up.
And yeah, it looked like a bug that worth to fixing.

> Let's not be too hard on him for trying; I appreciate it more
> than spelling fixes and whitespace cleanups.  ;)
>
> I agree that some Coverity CIDs are false, and we shouldn't
> mangle code just to make it happy, but I just don't think that's
> what's going on here.  Let's imagine Geyslan saw 10 other CIDs
> and elected not to send changes, because they didn't look like
> they needed fixing, but this one looked like a bona fide bug.
>
Yep.

>> Asking the question may not change the outcome, but we need to ask
>> and answer the question regardless.
>
>>> (We don't have to change code to shut up coverity; we can flag
>>> it in the database and nobody else will see it.)
>>
>> Only if you are the first to see it and make an executive decision
>> that it's not necessary to fix.... :/
>
> Or, you find it, send a patch that seems reasonable, get massive
> pushback, (hopefully) flag it, and resolve never come back to xfs
> again.  ;)
Really, FWIW, the whole discussion to me is somewhat prolix. Let's see:

- We have a Coverity spot (Dereference after NULL check) = BUG.
- You identified that was intentional and isn't a bug. Ok?
- Regarding the "possible new patch" subject, I humbly pass the ball to you.

Thank you for the attention.

Geyslan
Regards.
>
> -Eric
>
>> Cheers,
>>
>> Dave.
>>
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/22 Eric Sandeen <sandeen@sandeen.net>:
> On 10/22/13 4:03 PM, Dave Chinner wrote:
>> On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
>>> On 10/22/13 3:39 PM, Dave Chinner wrote:
>>>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
>>>>> 2013/10/21 Dave Chinner <david@fromorbit.com>:
>>>>>> 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:
>>>>>>
>>>>>> 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).
>>>>
>>>> You should have put that in the patch description.
>>>>
>>>> Now I understand why there's been a sudden surge of irrelevant one
>>>> line changes from random people that have never touched XFS before.
>>>>
>>>> <sigh>
>>>>
>>>> Ok, lets churn the code just to shut the stupid checker up. This
>>>> doesn't fix a bug, it doesn't change behaviour, it just makes
>>>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
>>>> in a previous message.
>>>
>>> You know, I respectfully disagree, but we might just have to agree
>>> to disagree.  The code, as it stands, tests for a null ptr
>>> and then dereferences it.  That's always going to raise some
>>> eyebrows, coverity or not, debug code or not, drive by or not.
>>
>>> So even for future developers, making the code more self-
>>> documenting about this behavior would be a plus, whether it's by
>>> comment, by explicit ASSERT(), or whatever.  (I don't think
>>> that xfs_emerg() has quite enough context to make it obvious.)
>>
>> Sure, but if weren't for the fact that Coverity warned about it,
>> nobody other that us people who work on the XFS code day in, day out
>> would have even cared about it.
>>
>> That's kind of my point - again, as the Devil's Advocate - that
>> coverity is encouraging drive-by "fixes" by people who don't
>> actually understand any of the context, history and/or culture
>> surrounding the code being modified.
>
Dave, If Coverity had not caught this, I could have never sent a patch
to xfs in my entire life.
So, I need not to know the xfs culture, code or context to identify a
flagrant, intentional or not, code that seems a bug.
Open source world works this way, all can help, but only a few decide
to do it. And there many ways to do it; static analysis is only one.

> They shouldn't have to, the code (or comments therein) should
> make it obvious.  ;)  (in a perfect world...)
>
>> I have no problems with real bugs being fixed, but if we are
>> modifying code for no gain other than closing "coverity doesn't like
>> it" bugs, then we *should* be questioning whether the change is
>> really necessary.
>
> But let's give Geyslan the benefit of the doubt, and realize that
> Coverity does find real things, and even if it originated w/ a
> Coverity CID, when one sees:
>
>         if (!a)
>                 printk("a thing\n")
>
>         a = a->b = . . .
>
> it looks suspicious to pretty much anyone.  I don't think Geyslan
> sent it to shut Coverity up, he sent it because it looked like
> a bug worth fixing (after Coverity spotted it).
>
Eric, you're right. In this particular case, I didn't send to shut Coverity up.
And yeah, it looked like a bug that worth to fixing.

> Let's not be too hard on him for trying; I appreciate it more
> than spelling fixes and whitespace cleanups.  ;)
>
> I agree that some Coverity CIDs are false, and we shouldn't
> mangle code just to make it happy, but I just don't think that's
> what's going on here.  Let's imagine Geyslan saw 10 other CIDs
> and elected not to send changes, because they didn't look like
> they needed fixing, but this one looked like a bona fide bug.
>
Yep.

>> Asking the question may not change the outcome, but we need to ask
>> and answer the question regardless.
>
>>> (We don't have to change code to shut up coverity; we can flag
>>> it in the database and nobody else will see it.)
>>
>> Only if you are the first to see it and make an executive decision
>> that it's not necessary to fix.... :/
>
> Or, you find it, send a patch that seems reasonable, get massive
> pushback, (hopefully) flag it, and resolve never come back to xfs
> again.  ;)
Really, FWIW, the whole discussion to me is somewhat prolix. Let's see:

- We have a Coverity spot (Dereference after NULL check) = BUG.
- You identified that was intentional and isn't a bug. Ok?
- Regarding the "possible new patch" subject, I humbly pass the ball to you.

Thank you for the attention.

Geyslan
Regards.
>
> -Eric
>
>> Cheers,
>>
>> Dave.
>>
>

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

Hey Geyslan,

On Wed, Oct 23, 2013 at 08:58:51AM -0200, Geyslan Gregório Bem wrote:
> - Regarding the "possible new patch" subject, I humbly pass the ball to you.
> 
> Thank you for the attention.

Thank you for the patch.  I would really prefer to commit this showing
authorship from you, rather than a Reported-by.  Can I mark you down?

Regards,
	Ben

---

xfs: fix possible NULL dereference in xlog_verify_iclog

In xlog_verify_iclog a debug check of the incore log buffers prints an
error if icptr is null and then goes on to dereference the pointer
regardless.  Convert this to an assert so that the intention is clear.
This was reported by Coverty.

Reported-by: Geyslan G. Bem <geyslan@gmail.com>
Signed-off-by: Ben Myers <bpm@sgi.com>
---
 fs/xfs/xfs_log.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: b/fs/xfs/xfs_log.c
===================================================================
--- a/fs/xfs/xfs_log.c	2013-10-23 14:52:47.875216875 -0500
+++ b/fs/xfs/xfs_log.c	2013-10-23 14:53:53.775245830 -0500
@@ -3714,11 +3714,9 @@ xlog_verify_iclog(
 	/* check validity of iclog pointers */
 	spin_lock(&log->l_icloglock);
 	icptr = log->l_iclog;
-	for (i=0; i < log->l_iclog_bufs; i++) {
-		if (icptr == NULL)
-			xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
-		icptr = icptr->ic_next;
-	}
+	for (i=0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
+      		ASSERT(icptr);
+
 	if (icptr != log->l_iclog)
 		xfs_emerg(log->l_mp, "%s: corrupt iclog ring", __func__);
 	spin_unlock(&log->l_icloglock);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

Hey Geyslan,

On Wed, Oct 23, 2013 at 08:58:51AM -0200, Geyslan Gregório Bem wrote:
> - Regarding the "possible new patch" subject, I humbly pass the ball to you.
> 
> Thank you for the attention.

Thank you for the patch.  I would really prefer to commit this showing
authorship from you, rather than a Reported-by.  Can I mark you down?

Regards,
	Ben

---

xfs: fix possible NULL dereference in xlog_verify_iclog

In xlog_verify_iclog a debug check of the incore log buffers prints an
error if icptr is null and then goes on to dereference the pointer
regardless.  Convert this to an assert so that the intention is clear.
This was reported by Coverty.

Reported-by: Geyslan G. Bem <geyslan@gmail.com>
Signed-off-by: Ben Myers <bpm@sgi.com>
---
 fs/xfs/xfs_log.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: b/fs/xfs/xfs_log.c
===================================================================
--- a/fs/xfs/xfs_log.c	2013-10-23 14:52:47.875216875 -0500
+++ b/fs/xfs/xfs_log.c	2013-10-23 14:53:53.775245830 -0500
@@ -3714,11 +3714,9 @@ xlog_verify_iclog(
 	/* check validity of iclog pointers */
 	spin_lock(&log->l_icloglock);
 	icptr = log->l_iclog;
-	for (i=0; i < log->l_iclog_bufs; i++) {
-		if (icptr == NULL)
-			xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
-		icptr = icptr->ic_next;
-	}
+	for (i=0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
+      		ASSERT(icptr);
+
 	if (icptr != log->l_iclog)
 		xfs_emerg(log->l_mp, "%s: corrupt iclog ring", __func__);
 	spin_unlock(&log->l_icloglock);

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/23 Ben Myers <bpm@sgi.com>:
> Hey Geyslan,
>
> On Wed, Oct 23, 2013 at 08:58:51AM -0200, Geyslan Gregório Bem wrote:
>> - Regarding the "possible new patch" subject, I humbly pass the ball to you.
>>
>> Thank you for the attention.
>
> Thank you for the patch.  I would really prefer to commit this showing
> authorship from you, rather than a Reported-by.  Can I mark you down?
>
> Regards,
>         Ben
>
Thank you, Ben. Sure, you can mark me.

> ---
>
> xfs: fix possible NULL dereference in xlog_verify_iclog
>
> In xlog_verify_iclog a debug check of the incore log buffers prints an
> error if icptr is null and then goes on to dereference the pointer
> regardless.  Convert this to an assert so that the intention is clear.
> This was reported by Coverty.
>
> Reported-by: Geyslan G. Bem <geyslan@gmail.com>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
>  fs/xfs/xfs_log.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> Index: b/fs/xfs/xfs_log.c
> ===================================================================
> --- a/fs/xfs/xfs_log.c  2013-10-23 14:52:47.875216875 -0500
> +++ b/fs/xfs/xfs_log.c  2013-10-23 14:53:53.775245830 -0500
> @@ -3714,11 +3714,9 @@ xlog_verify_iclog(
>         /* check validity of iclog pointers */
>         spin_lock(&log->l_icloglock);
>         icptr = log->l_iclog;
> -       for (i=0; i < log->l_iclog_bufs; i++) {
> -               if (icptr == NULL)
> -                       xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> -               icptr = icptr->ic_next;
> -       }
> +       for (i=0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
> +               ASSERT(icptr);
> +
>         if (icptr != log->l_iclog)
>                 xfs_emerg(log->l_mp, "%s: corrupt iclog ring", __func__);
>         spin_unlock(&log->l_icloglock);
>

-- 
Regards,

Geyslan G. Bem
hackingbits.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/23 Ben Myers <bpm@sgi.com>:
> Hey Geyslan,
>
> On Wed, Oct 23, 2013 at 08:58:51AM -0200, Geyslan Gregório Bem wrote:
>> - Regarding the "possible new patch" subject, I humbly pass the ball to you.
>>
>> Thank you for the attention.
>
> Thank you for the patch.  I would really prefer to commit this showing
> authorship from you, rather than a Reported-by.  Can I mark you down?
>
> Regards,
>         Ben
>
Thank you, Ben. Sure, you can mark me.

> ---
>
> xfs: fix possible NULL dereference in xlog_verify_iclog
>
> In xlog_verify_iclog a debug check of the incore log buffers prints an
> error if icptr is null and then goes on to dereference the pointer
> regardless.  Convert this to an assert so that the intention is clear.
> This was reported by Coverty.
>
> Reported-by: Geyslan G. Bem <geyslan@gmail.com>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
>  fs/xfs/xfs_log.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> Index: b/fs/xfs/xfs_log.c
> ===================================================================
> --- a/fs/xfs/xfs_log.c  2013-10-23 14:52:47.875216875 -0500
> +++ b/fs/xfs/xfs_log.c  2013-10-23 14:53:53.775245830 -0500
> @@ -3714,11 +3714,9 @@ xlog_verify_iclog(
>         /* check validity of iclog pointers */
>         spin_lock(&log->l_icloglock);
>         icptr = log->l_iclog;
> -       for (i=0; i < log->l_iclog_bufs; i++) {
> -               if (icptr == NULL)
> -                       xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> -               icptr = icptr->ic_next;
> -       }
> +       for (i=0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
> +               ASSERT(icptr);
> +
>         if (icptr != log->l_iclog)
>                 xfs_emerg(log->l_mp, "%s: corrupt iclog ring", __func__);
>         spin_unlock(&log->l_icloglock);
>

-- 
Regards,

Geyslan G. Bem
hackingbits.com

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

On 10/23/13 3:34 PM, Ben Myers wrote:

> xfs: fix possible NULL dereference in xlog_verify_iclog
> 
> In xlog_verify_iclog a debug check of the incore log buffers prints an
> error if icptr is null and then goes on to dereference the pointer
> regardless.  Convert this to an assert so that the intention is clear.
> This was reported by Coverty.
> 
> Reported-by: Geyslan G. Bem <geyslan@gmail.com>
> Signed-off-by: Ben Myers <bpm@sgi.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_log.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> Index: b/fs/xfs/xfs_log.c
> ===================================================================
> --- a/fs/xfs/xfs_log.c	2013-10-23 14:52:47.875216875 -0500
> +++ b/fs/xfs/xfs_log.c	2013-10-23 14:53:53.775245830 -0500
> @@ -3714,11 +3714,9 @@ xlog_verify_iclog(
>  	/* check validity of iclog pointers */
>  	spin_lock(&log->l_icloglock);
>  	icptr = log->l_iclog;
> -	for (i=0; i < log->l_iclog_bufs; i++) {
> -		if (icptr == NULL)
> -			xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> -		icptr = icptr->ic_next;
> -	}
> +	for (i=0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
> +      		ASSERT(icptr);
> +
>  	if (icptr != log->l_iclog)
>  		xfs_emerg(log->l_mp, "%s: corrupt iclog ring", __func__);
>  	spin_unlock(&log->l_icloglock);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Eric Sandeen]

On 10/23/13 3:34 PM, Ben Myers wrote:

> xfs: fix possible NULL dereference in xlog_verify_iclog
> 
> In xlog_verify_iclog a debug check of the incore log buffers prints an
> error if icptr is null and then goes on to dereference the pointer
> regardless.  Convert this to an assert so that the intention is clear.
> This was reported by Coverty.
> 
> Reported-by: Geyslan G. Bem <geyslan@gmail.com>
> Signed-off-by: Ben Myers <bpm@sgi.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_log.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> Index: b/fs/xfs/xfs_log.c
> ===================================================================
> --- a/fs/xfs/xfs_log.c	2013-10-23 14:52:47.875216875 -0500
> +++ b/fs/xfs/xfs_log.c	2013-10-23 14:53:53.775245830 -0500
> @@ -3714,11 +3714,9 @@ xlog_verify_iclog(
>  	/* check validity of iclog pointers */
>  	spin_lock(&log->l_icloglock);
>  	icptr = log->l_iclog;
> -	for (i=0; i < log->l_iclog_bufs; i++) {
> -		if (icptr == NULL)
> -			xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> -		icptr = icptr->ic_next;
> -	}
> +	for (i=0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
> +      		ASSERT(icptr);
> +
>  	if (icptr != log->l_iclog)
>  		xfs_emerg(log->l_mp, "%s: corrupt iclog ring", __func__);
>  	spin_unlock(&log->l_icloglock);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

Hey Geyslan,

On Wed, Oct 30, 2013 at 03:08:12PM -0500, Eric Sandeen wrote:
> On 10/23/13 3:34 PM, Ben Myers wrote:
> 
> > xfs: fix possible NULL dereference in xlog_verify_iclog
> > 
> > In xlog_verify_iclog a debug check of the incore log buffers prints an
> > error if icptr is null and then goes on to dereference the pointer
> > regardless.  Convert this to an assert so that the intention is clear.
> > This was reported by Coverty.
> > 
> > Reported-by: Geyslan G. Bem <geyslan@gmail.com>
> > Signed-off-by: Ben Myers <bpm@sgi.com>
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Applied this.  Many thanks!  ;)

-Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Ben Myers]

Hey Geyslan,

On Wed, Oct 30, 2013 at 03:08:12PM -0500, Eric Sandeen wrote:
> On 10/23/13 3:34 PM, Ben Myers wrote:
> 
> > xfs: fix possible NULL dereference in xlog_verify_iclog
> > 
> > In xlog_verify_iclog a debug check of the incore log buffers prints an
> > error if icptr is null and then goes on to dereference the pointer
> > regardless.  Convert this to an assert so that the intention is clear.
> > This was reported by Coverty.
> > 
> > Reported-by: Geyslan G. Bem <geyslan@gmail.com>
> > Signed-off-by: Ben Myers <bpm@sgi.com>
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Applied this.  Many thanks!  ;)

-Ben

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/31 Ben Myers <bpm@sgi.com>:
> Hey Geyslan,
>
> On Wed, Oct 30, 2013 at 03:08:12PM -0500, Eric Sandeen wrote:
>> On 10/23/13 3:34 PM, Ben Myers wrote:
>>
>> > xfs: fix possible NULL dereference in xlog_verify_iclog
>> >
>> > In xlog_verify_iclog a debug check of the incore log buffers prints an
>> > error if icptr is null and then goes on to dereference the pointer
>> > regardless.  Convert this to an assert so that the intention is clear.
>> > This was reported by Coverty.
>> >
>> > Reported-by: Geyslan G. Bem <geyslan@gmail.com>
>> > Signed-off-by: Ben Myers <bpm@sgi.com>
>>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>
> Applied this.  Many thanks!  ;)
>
> -Ben

It was a pleasure. o/

-- 
Regards,

Geyslan G. Bem
hackingbits.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix possible NULL dereference

[Geyslan Gregório Bem]

2013/10/31 Ben Myers <bpm@sgi.com>:
> Hey Geyslan,
>
> On Wed, Oct 30, 2013 at 03:08:12PM -0500, Eric Sandeen wrote:
>> On 10/23/13 3:34 PM, Ben Myers wrote:
>>
>> > xfs: fix possible NULL dereference in xlog_verify_iclog
>> >
>> > In xlog_verify_iclog a debug check of the incore log buffers prints an
>> > error if icptr is null and then goes on to dereference the pointer
>> > regardless.  Convert this to an assert so that the intention is clear.
>> > This was reported by Coverty.
>> >
>> > Reported-by: Geyslan G. Bem <geyslan@gmail.com>
>> > Signed-off-by: Ben Myers <bpm@sgi.com>
>>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>
> Applied this.  Many thanks!  ;)
>
> -Ben

It was a pleasure. o/

-- 
Regards,

Geyslan G. Bem
hackingbits.com