Hi there,
On Thu, Jun 03, 2004 at 03:10:09PM -0700, David S. Miller wrote:
> On Thu, 3 Jun 2004 15:04:28 -0700 (PDT)
> Linus Torvalds <torvalds@xxxxxxxx> wrote:
>
> [ Nathan, the issue is that XFS calls ffs() potentially with an arg
> value of zero which is illegal and has undefined behavior, the
> XFS code is depending upon ffs() returning -1 in this case. ]
Hmm. We will pass in zero in a couple places (see below),
and we are indeed relying on the behaviour documented for
userspace ffs there which is that ffs returns zero if its
given zero.
> > On Thu, 3 Jun 2004, David S. Miller wrote:
> > >
> > > So what's the deal? Is it supposed to return -1 when being passed
> > > zero or is it supposed to act, as per the comments above each
> > > implementation, like the userland ffs manpage states in such a
> > > case?
> >
> > It's undefined behaviour.
> >
> > If you do ffs() without checking the value, you get what you get.
>
> Ok, here are the potential culprits under fs/xfs:
From a quick look through these...
> ./linux-2.6/xfs_buf.c: btp->pbr_sshift = ffs(sectorsize) - 1;
'sokay, sectorsize is always a 512 byte multiple >= 512. This is
guarenteed (bounds checked) at mount time, after reading the sb.
> ./linux-2.6/xfs_super.c: sb->s_blocksize_bits = ffs(statvfs.f_bsize) - 1;
Likewise, blocksize is always a 512 byte multiple >= 512.
> ./xfs_bit.c: n = ffs((unsigned)v);
> ./xfs_bit.c: n = ffs(v >> 32);
These two are in xfs_lowbit64, and they are relying on the zero-
returned-on-zero behaviour. After staring at it, I think this
code has slightly suspect handling of the high 32 bits - It does
seem to have a somewhat confused notion of the return value.
So, I think I'll need to rework parts of this routine anyway,
and can add a guard against a zero input value here if needed.
> ./xfs_bit.c: return result + ffs(tmp) - 1;
"tmp" is guaranteed to be non-zero in this context.
> ./xfs_dir2_data.h: * Offsets of . and .. in data space (always block 0)
> ./xfs_dir2_leaf.h: * Offset of the leaf/node space. First block in this space
> ./xfs_dir2_leaf.h: * Offset in data space of a data entry.
> ./xfs_dir2_node.h: * Offset of the freespace index.
Heh.
> ./xfs_log_recover.c: bufblks = 1 << ffs(nbblks);
> ./xfs_log_recover.c: bufblks = 1 << ffs(blocks);
These two are allocating buffers for some number of blocks, and
all callers guarantee this value to be non-zero.
> ./xfs_rtalloc.c: return ffs(v)-1;
All callers of this guy (xfs_lowbit32, aka XFS_RTLOBIT) also
guarantee that the passed in value is non-zero.
So, looks like I need to fix up those two callers in xfs_lowbit64
and XFS will be safe independent of the ffs behavior on zero.
thanks.
--
Nathan
|