Date: Sun, 3 Feb 2013 18:32:34 -0800 (PST) From: Pedro Giffuni <pfg@freebsd.org> To: Bruce Evans <brde@optusnet.com.au>, Andriy Gapon <avg@FreeBSD.org> Cc: "freebsd-fs@freebsd.org" <freebsd-fs@freebsd.org>, Kirk McKusick <mckusick@FreeBSD.org> Subject: Re: svn commit: r246289 - head/sys/ufs/ffs Message-ID: <1359945154.62069.YahooMailNeo@web162105.mail.bf1.yahoo.com> In-Reply-To: <20130204062149.U2673@besplex.bde.org> References: <201302031716.r13HGXNP060303@svn.freebsd.org> <510E9D47.2030403@FreeBSD.org> <20130204062149.U2673@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
(Moving the discussion to freebsd-fs)=0A=0AHello;=0A=0A=0A----- Messaggio o= riginale -----=0A> Da: Bruce Evans=A0=0A> =0A...=0A>> Just a note that cla= ng actually warned about this one.=0A>> It has a few more similar warnings= for ufs/ffs code.=0A> =0A> I wondered how the DIP macro hid the warning.= =0A>=A0=0A=0AThe comparison is perfectly legal for UFS1 so perhaps=0Agcc gi= ves the "benefit of the doubt" to avoid false positives.=0A=0A>>> =A0 bloc= ks being freed against the value i_blocks. If the number of=0A>>> =A0 bloc= ks being freed exceeds i_blocks, just set i_blocks to zero.=0A>>> =0A>>> = =A0 Reported by: Pedro Giffuni (pfg@)=0A>>> =A0 MFC after:=A0 2 weeks=0A= > =0A> Perhaps the larger bugs pointed to this warning were lost in transla= tion:=0A> - di_blocks overflows for ffs1.=A0 This is now physically possibl= e.=0A> =A0 di_blocks has type int32_t, as required to match st_blocks in th= e old=0A> =A0 struct stat (both will overflow at the same point).=A0 Since = di_blocks=0A> =A0 counts DEV_BSIZE-blocks, overflow occurs at file size abo= ut 1TB for=0A> =A0 files without many holes.=0A> - st_blocks overflows for = the old stat() syscall.=A0 For the new stat()=0A> =A0 syscall, st_blocks ha= s type blkcnt_t =3D int64_t, so overflow is not=0A> =A0 physically possible= .=A0 But cvtstat() silently overflows when the=0A> =A0 64-bit st_blocks doe= sn't fit in the 32-bit one.=0A> - di_blocks for ffs2 has type uint64_t.=A0 = This has a sign mismatch with=0A> =A0 both the ffs1 di_blocks and blkcnt_t.= =A0 blkcnt_t is specified to be=0A> =A0 signed.=A0 i_blocks for ffs has the= same type as di_blocks for ffs2=0A> =A0 (unsigned ...), so it is mismatche= d too.=A0 The sign mismatches should=0A> =A0 cause more compiler warnings.= =A0 These would point to further overflow=0A> =A0 possibilities.=A0 However= , overflow is physically impossible even for=0A> =A0 va_bytes =3D i_blocks = * DEV_BSIZE.=A0 So there are no extra overflows=0A> =A0 from the sign misma= tches.=A0 Using the unsigned types just asks for=0A> =A0 more sign extensio= n bugs like the one fixed here.=0A> =0A> ext2fs has the same bug.=A0 It is = unnecessary in ext2fs, since there are=0A> no fragments so i_blocks can jus= t count in units of fs blocks and these=0A> are naturally limited by the fs= block type.=A0 The fs block type is=0A> uint32_t in ext2fs and int32_t in = ffs1.=A0 So ext2fs really needs an=0A> unsigned type for i_blocks to get an= extra bit without going to 64=0A> bits.=A0 shell-init: could not get curre= nt directory: No such file or=0A> directory.=A0 Avoiding overflow depends o= n st_blocks having more than 32+9=0A> value bits, so 32-bit stat() could st= ill overflow and needs a clamp.=0A> However, the disk format is to store i_= count in DEV_BSIZE units, so=0A> this doesn't work -- it has the same overf= low problem as cvtstat(),=0A> at 32 bits instead of 31.=0A> =0A> Limiting t= he file size to ~1TB is not a good fix for this, since 1TB=0A> non-sparse f= iles are not very common or useful, and the limit would=0A> also apply to s= parse files.=A0 So block allocators should check that=0A> i_blocks won't ov= erflow before increasing it.=0A>=A0=0A=0ASurely not anywhere near a complet= e solution but perhaps it wouldn't=0Abe incompatible=A0to change i_blocks a= nd friends to be unsigned in UFS1.=0AThat is=A0something that remains to be= completed in ext2fs, but according=0Ato fsx there=A0are bigger=A0problems = there at this time.=0A=0APedro.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1359945154.62069.YahooMailNeo>