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

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

dholland@ wrote:

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

Yes, but it is not implementation issue but design issue.

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

Yes, but which one? getdisksize() or DIOCGPART in each driver?

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

It depends how the design issue is resolved.
If DIOCGPART must not return secsize=0,
KASSERT in getdisksize() seems reasonable.
If uninitialized secsize=0 from DIOCGPART is acceptable,
getdisksize() or each caller should have sanity checks.
---
Izumi Tsutsui