Re: CVS commit: src/sys/fs/msdosfs

[David Holland <dholland-sourcechanges%netbsd.org@localhost>]

On Tue, Jul 03, 2012 at 08:01:21PM +0900, Izumi Tsutsui wrote:
 > > } Add a sanity check if secsize returned from getdisksize() isn't bogus.
 > > } This prevent possible panic "panic: buf mem pool index 23" later in
 > > } vfs_bio.c:buf_mempoolidx().
 > > } (I'm not sure if it's okay for getdisksize() to assume that
 > > }  partinfo taken from DIOCGPART is properly initialized
 > > }  on all disk(9) devices or not)
 > > 
 > >      I think a better question is, why is getdisksize() returning
 > > garbage instead of returning an error?
 > 
 > I don't know because there is no getdisksize(9) man page.
 > Probably we have to ask the author of it.

getdisksize() is not complicated; it tries DIOCGPART and then
DIOCGWEDGEINFO and that's about it.

I think it's wrong for either of those to be returning a size of 0.

 > >      If this check is really necessary, I would be more inclined to use
 > > KASSERT().
 > 
 > I can change it to KASSERT if we can assume that partinfo taken
 > from DIOCGPART must be initialized properly on all disk(9) devices,
 > i.e. if we can say disklab->d_secsize==0 means driver's bug,
 > but I could not find any evidence.
 >
 > All vfs mountfs functions are called in case of "root on generic"
 > via mountroot in setroot() so I thought returning error was better
 > than panic in that case, even after md(4) was fixed.
 > But if you don't agree it, please revert it.

If we know it can happen, crashing isn't very productive.

However, can you move the test to inside getdisksize()? Make it print
a message and return EINVAL (or something). That way things other than
msdosfs are also protected.

-- 
David A. Holland
dholland%netbsd.org@localhost