filldir[64] oddness

filldir[64] oddness

[Dave Jones]

I'm puzzled by an aparent use of uninitialised memory
that coverity's checker picked up.

fs/readdir.c

#define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de)))
#define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1))

140  	static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
141  			   ino_t ino, unsigned int d_type)
142  	{
143  		struct linux_dirent __user * dirent;
144  		struct getdents_callback * buf = (struct getdents_callback *) __buf
145  		int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2);

How come that NAME_OFFSET isn't causing an oops when
it dereferences stackjunk->d_name ?

		Dave

-- 
http://www.codemonkey.org.uk

Re: filldir[64] oddness

[David S. Miller]

From: Dave Jones <davej@redhat.com>
Date: Wed, 8 Mar 2006 23:27:44 -0500

> #define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de)))
> #define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1))
> 
> 140  	static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
> 141  			   ino_t ino, unsigned int d_type)
> 142  	{
> 143  		struct linux_dirent __user * dirent;
> 144  		struct getdents_callback * buf = (struct getdents_callback *) __buf
> 145  		int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2);
> 
> How come that NAME_OFFSET isn't causing an oops when
> it dereferences stackjunk->d_name ?

d_name a char[] array, and we're just doing pointer arithmetic
here.  It's the same as "offsetof(d_name, struct linux_dirent)"
or something like that.

I think coverity is being trigger happy in this case :-)

Re: filldir[64] oddness

[Vadim Lobanov]

On Wed, 8 Mar 2006, Dave Jones wrote:

> I'm puzzled by an aparent use of uninitialised memory
> that coverity's checker picked up.
>
> fs/readdir.c
>
> #define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de)))
> #define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1))
>
> 140  	static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
> 141  			   ino_t ino, unsigned int d_type)
> 142  	{
> 143  		struct linux_dirent __user * dirent;
> 144  		struct getdents_callback * buf = (struct getdents_callback *) __buf
> 145  		int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2);
>
> How come that NAME_OFFSET isn't causing an oops when
> it dereferences stackjunk->d_name ?

If I had to guess...

Because NAME_OFFSET(de) is essentially doing
	de->d_name - de
which should be equivalent to just the static offset of d_name within
struct linux_direct. Which should be constant, no? Why does it need to
pass a pointer to compute this? Who knows.

> 		Dave
>
> --
> http://www.codemonkey.org.uk

- Vadim Lobanov

Re: filldir[64] oddness

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Wed, 8 Mar 2006 23:27:44 -0500 Dave Jones <davej@redhat.com> wrote:
>
> I'm puzzled by an aparent use of uninitialised memory
> that coverity's checker picked up.
> 
> fs/readdir.c
> 
> #define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de)))
> #define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1))
> 
> 140  	static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
> 141  			   ino_t ino, unsigned int d_type)
> 142  	{
> 143  		struct linux_dirent __user * dirent;
> 144  		struct getdents_callback * buf = (struct getdents_callback *) __buf
> 145  		int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2);
> 
> How come that NAME_OFFSET isn't causing an oops when
> it dereferences stackjunk->d_name ?

because d_name is an array, so the reference is really &(dirent->d_name[0]) and no
dereference occurs ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: filldir[64] oddness

[Al Viro]

On Wed, Mar 08, 2006 at 11:27:44PM -0500, Dave Jones wrote:
> I'm puzzled by an aparent use of uninitialised memory
> that coverity's checker picked up.
> 
> fs/readdir.c
> 
> #define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de)))
> #define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1))
> 
> 140  	static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
> 141  			   ino_t ino, unsigned int d_type)
> 142  	{
> 143  		struct linux_dirent __user * dirent;
> 144  		struct getdents_callback * buf = (struct getdents_callback *) __buf
> 145  		int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2);
> 
> How come that NAME_OFFSET isn't causing an oops when
> it dereferences stackjunk->d_name ?

It doesn't dereference it.  d_name is an array, not a pointer.  FWIW,
it should've been

#define NAME_OFFSET(de) offsetof(typeof(de), d_name)

or, better yet

#define NAME_OFFSET offsetof(struct linux_dirent, d_name)
#define NAME_OFFSET64 offsetof(struct linux_dirent64, d_name)

Re: filldir[64] oddness

[Dave Jones]

On Wed, Mar 08, 2006 at 08:32:04PM -0800, David S. Miller wrote:
 > From: Dave Jones <davej@redhat.com>
 > Date: Wed, 8 Mar 2006 23:27:44 -0500
 > 
 > > #define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de)))
 > > #define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1))
 > > 
 > > 140  	static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
 > > 141  			   ino_t ino, unsigned int d_type)
 > > 142  	{
 > > 143  		struct linux_dirent __user * dirent;
 > > 144  		struct getdents_callback * buf = (struct getdents_callback *) __buf
 > > 145  		int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2);
 > > 
 > > How come that NAME_OFFSET isn't causing an oops when
 > > it dereferences stackjunk->d_name ?
 > 
 > d_name a char[] array, and we're just doing pointer arithmetic
 > here.  It's the same as "offsetof(d_name, struct linux_dirent)"
 > or something like that.

Duh, yes. Of course.

 > I think coverity is being trigger happy in this case :-)

agreed.

		Dave

-- 
http://www.codemonkey.org.uk

Re: filldir[64] oddness

[Al Viro]

On Wed, Mar 08, 2006 at 08:32:04PM -0800, David S. Miller wrote:

> I think coverity is being trigger happy in this case :-)

If that's coverity, I'm very disappointed and more than a little
suspicious about the quality of their results.  This sort of
misparsing is easily made by human reader, but anything doing
type analysis must handle that case without any problems.

Re: filldir[64] oddness

[Bryan O'Sullivan]

On Thu, 2006-03-09 at 04:40 +0000, Al Viro wrote:
> On Wed, Mar 08, 2006 at 08:32:04PM -0800, David S. Miller wrote:
> 
> > I think coverity is being trigger happy in this case :-)
> 
> If that's coverity, I'm very disappointed and more than a little
> suspicious about the quality of their results.

About half of the ~50 reports I've looked at so far in their database
have been false positives.  In most of those cases, it's not obvious how
a checker might have gotten them right instead, though.

	<b

Re: filldir[64] oddness

[Dave Jones]

On Thu, Mar 09, 2006 at 09:02:22AM -0800, Bryan O'Sullivan wrote:
 > On Thu, 2006-03-09 at 04:40 +0000, Al Viro wrote:
 > > On Wed, Mar 08, 2006 at 08:32:04PM -0800, David S. Miller wrote:
 > > 
 > > > I think coverity is being trigger happy in this case :-)
 > > 
 > > If that's coverity, I'm very disappointed and more than a little
 > > suspicious about the quality of their results.
 > 
 > About half of the ~50 reports I've looked at so far in their database
 > have been false positives.  In most of those cases, it's not obvious how
 > a checker might have gotten them right instead, though.

It seems to stumble quite a bit when faced with things that are
free'd when refcounts drop to zero. (skbs, and kobjects).

		Dave

-- 
http://www.codemonkey.org.uk

Re: filldir[64] oddness

[Bryan O'Sullivan]

On Thu, 2006-03-09 at 12:07 -0500, Dave Jones wrote:

>  > About half of the ~50 reports I've looked at so far in their database
>  > have been false positives.  In most of those cases, it's not obvious how
>  > a checker might have gotten them right instead, though.
> 
> It seems to stumble quite a bit when faced with things that are
> free'd when refcounts drop to zero. (skbs, and kobjects).

Yes, or (in my case) stuff like "when this variable has value X, that
pointer can't possibly be NULL".

	<b

Re: filldir[64] oddness

[Dave Jones]

On Thu, Mar 09, 2006 at 09:15:14AM -0800, Bryan O'Sullivan wrote:
 > On Thu, 2006-03-09 at 12:07 -0500, Dave Jones wrote:
 > 
 > >  > About half of the ~50 reports I've looked at so far in their database
 > >  > have been false positives.  In most of those cases, it's not obvious how
 > >  > a checker might have gotten them right instead, though.
 > > 
 > > It seems to stumble quite a bit when faced with things that are
 > > free'd when refcounts drop to zero. (skbs, and kobjects).
 > 
 > Yes, or (in my case) stuff like "when this variable has value X, that
 > pointer can't possibly be NULL".

*nod*
It does call into question the "OMFG, there are 1000 bugs in the kernel"
hysteria that has found its way through various news forums.
The genuine bugs it does find are gold dust though.  There's a bunch
of stuff that's sat there for an eternity. It's just painstaking to
grovel through the reports weeding out the false positives.

A lot of the 'bugs' it's found are also not really going to make
the world stop turning soon. It even picked up a few cases of
code doing like.

void foo()
{
	int x;

	while (read status from hardware reg != READY)
		x++;
}

as uninitialised. Which is true, but as there's nothing
dependant on it, it's harmless.

		Dave

-- 
http://www.codemonkey.org.uk

Re: filldir[64] oddness

[Kyle Moffett]

On Mar 9, 2006, at 12:02:22, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 04:40 +0000, Al Viro wrote:
>> On Wed, Mar 08, 2006 at 08:32:04PM -0800, David S. Miller wrote:
>>> I think coverity is being trigger happy in this case :-)
>>
>> If that's coverity, I'm very disappointed and more than a little  
>> suspicious about the quality of their results.
>
> About half of the ~50 reports I've looked at so far in their  
> database have been false positives.  In most of those cases, it's  
> not obvious how a checker might have gotten them right instead,  
> though.

Yeah, IMHO it's not really worth optimizing for the obscure and oddly- 
defined cases unless you can actually find valid places where that  
code comes up understandably.  In this particular case, the Coverity  
checker is indirectly pointing out that the code is confusing to the  
reader and could inadvertently be massively broken by changing the  
type of d_name.

Cheers,
Kyle Moffett

Re: filldir[64] oddness

[Al Viro]

On Thu, Mar 09, 2006 at 08:41:08PM -0500, Kyle Moffett wrote:
> Yeah, IMHO it's not really worth optimizing for the obscure and oddly- 
> defined cases unless you can actually find valid places where that  
> code comes up understandably.  In this particular case, the Coverity  
> checker is indirectly pointing out that the code is confusing to the  
> reader and could inadvertently be massively broken by changing the  
> type of d_name.

Bullshit.  It is very directly pointing out that it has broken handling
of C types (obscure case, my arse - decay of arrays to pointers), has no
regression testsuite and most likely doesn't even get applied to its own
source on a regular basis.