> Juergen, this has been filed as:
>
> 6384346 ufsops.c reads from uninitialized stack
>
> against kernel/boot, since this belongs to the system startup
> code and not the generic UFS code.
To clarify on that, this code never deals with multithreaded
access and never deals with more than one filesystem. It's not
part of the kernel, it's only in ufsboot, and used by the BOP_*()
interfaces during boot for accessing the boot filesystem, before
loadrootmodules() is complete and BOP_QUIESCE_IO() is called by
the kernel. Likewise, it never calls the kernel's "kmem_alloc"
even if the code says so (gets bounced to krtld`bkmem_alloc),
hence it can't leak kernel memory ...
The chance of breakage here is tiny although it might explain
some kinds of boot failures on SPARC platforms. Matching this
to an actual failure wouldn't be for the faint-hearted, though :(
Now, going through the history of this in detail, I severely
wonder about 6341645. Why is this code even in krtld ? It's
not on SPARC, and it wasn't on x86 before the newboot changes,
so the only possible explanation left is that the newboot mods
actually now link the standalone libufs.a into krtld to provide
access to the boot archive. Is that right ? We're not calling
into Grub's ufs reader to parse the bootarchive contents, but
still into standalone libufs.a ?
<personal opinion>
Oh crud. The bootarchive should better have been a cpio/tar/zip/...
archive than a filesystem image ... there are too many filesystem
implementations involved in booting Solaris anyway ...
</personal opinion>
I'm burnt by this. Sorry for being blunt here. I seem to have a
talent for finding places where ages-old code/design can hurt
you when you work with it ...
Earlier on, when Solaris/x86 still had boot.bin, the symbols
were stripped out so namespace clashes could never occur. And,
as mentioned, the code wasn't permanent then either... but
would be thrown out when unmapping the boot loader's presence.
I need some divine power to stretch my workday to 48 hours so
that I can follow all developments closely :(
One more thing: If the above assertion (that krtld does NOT call
into grub for filesystem services, but into libufs.a) is not
correct, then the proper fix for 6341645 would be NOT to link
krtld against that. Tell me I'm wrong about the above, please :)
Best wishes,
FrankH.
>
> ---
> frankB
>
> On Fri, 09 Dec 2005 17:43:30 +0100, Jürgen Keil <[EMAIL PROTECTED]> wrote:
>
> > snv_28 contains a fix for bug 6341645: "global variable ('b') name
> > clashes with hex
> > digit on x86 mdb -k".
> >
> > The source file usr/src/common/fs/ufsops.c was changed to move
> > two global variables (variables |b| and |blknos|) into the function
> > sbmap(). Complete diff for the change is this:
> >
> >
http://cvs.opensolaris.org/source/diff/on/usr/src/common/fs/ufsops.c?r2=1.3&r1=1
.2
> >
> > Note that the global variable |b|, defined as "union { ... } b", had a
> > static attribute,
> > while global |blknos| had *no* static attribute. The static /
> > non-static attributes
> > for |b| and |blknos| are still in use, now that both variables have been
> > moved
> > into the sbmap() function.
> >
> > Looking at the code in sbmap(), isn't there now an access to an
> > uninitialized
> > local array element, at line 270? That doesn't look correct.
> >
> > 213 static daddr32_t
> > 214 sbmap(fileid_t *filep, daddr32_t bn)
> > 215 {
> > ...
> > 222 /* These are the pools of buffers, etc. */
> > 223 /* Compilers like to play with alignment, so force the issue
> > here */
> > 224 static union {
> > 225 char *blk[NIADDR + 1];
> > 226 daddr32_t *dummy;
> > 227 } b;
> > 228 daddr32_t blknos[NIADDR + 1];
> > ...
> > 266 /*
> > 267 * fetch through the indirect blocks
> > 268 */
> > 269 for (; j <= NIADDR; j++) {
> > 270 if (blknos[j] != nb) {
> >
> >
> >
> > To get back the original behaviour (that existed before fix 6341645
> > was applied), the local variable |blknos| should/must be declared with
> > attribute static.
> >
> >
> > I'm not sure what breaks due to this bug; I assume there could be
> >
> > - kernel panics when reading from big files using ufs indirect blocks
> > (uninitialized b.blk[] pointers are referenced)
> > - maybe performance problems when reading huge ufs files, because
> > the indirect block cache in sbmap does not work as expected.
> >
> >
> > Btw.: what happens when there are
> > - two ufs filesystems (ufs1, ufs2)
> > - both ufs filesystem contain an inode with indirect block (ino1 in ufs1,
> > ino2 in ufs2)
> > - the block number for an indirect block for ufs1/ino1 and ufs2/ino2
> > happens
> > to be the same for the two inodes (bn1 == bn2 == bn)
> > (but since they are located in different ufs filesystem, the indirect
> > block data
> > could point to different block numbers)
> >
> > and we're reading first from ino1: blknos[x] is set to bn1, b.blk[x]
> > points to
> > ino1's indirect block data;
> >
> > and now we're trying to read from ino2. blknos[x] would already contain
> > the correct
> > block number bn2 for ino2's indirect data block, and we would end up
> > using
> > ino1's cached indirect block map data when reading ino2 data!
> >
> > Did I miss something? Shouldn't the key for the indirect block cache in
> > sbmap()
> > include a reference to the ufs filesystem, in addition to the block
> > number?
> > This message posted from opensolaris.org
> > _______________________________________________
> > ufs-discuss mailing list
> > [email protected]
>
>
> _______________________________________________
> ufs-discuss mailing list
> [email protected]
_______________________________________________
ufs-discuss mailing list
[email protected]
