Created attachment 173646 [details] match types for comparison Comparing (int16_t)cgp->cg_old_niblk with (uint32_t)sblock.fs_ipg causes this test to fail on my fs (created on OpenBSD) resulting in the fs being marked dirty. The value being compared in this case is 52224. Judging by bug #174028 I'd imagine there are more issues of this nature lurking around. This diff was tested and generated against releng/10.3 but a quick look at the code in HEAD suggests it should still be an issue there.
I do not see how this patch can be right. You strip off high 16 bits from the superblock' fs_ipg value and then compare truncated value to UFS1' niblk cylgroup value. This is plain wrong. For UFS1, fs_ipg must be less than 2^15, so the values fit. I have no idea what OpenBSD does there, but this is not how FreeBSD UFS operates. I suspect that binary layout of superblock or cg header on OpenBSD is different.
This patch may very well be wrong since I have no knowledge of UFS internals but I assumed since these variables were being compared that the values were compatible (the value of sblock.fs_ipg able to fit in a data type the size of cgp->cg_old_niblk). I don't see how comparing int16_t to uint32_t can be right :) Would you prefer to cast cgp->cg_old_niblk to uint32_t? As far as I could tell with a brief look at their source, OpenBSD doesn't even compare these variables.
(In reply to Christian Heckendorf from comment #2) I prefer to keep the code as is. The cast is not needed, C compiler does silent standard-mandated promotion of the integer variables to the same rank and signess, in this case to uint32_t. So everything works as intended.
If the value in cgp->cg_old_niblk happens to be negative then you'll end up with a mess, or am I mistaken? (gdb) p (uint32_t)(int16_t)52224 $1 = 4294953984
If the cg_old_niblk value happens to be negative, it is invalid. How does this would wreak havoc, assuming a reasonable value in fs_ipg ?
Both variables hold the same value in their internal representation (0xcc00) but cg_old_niblk overflows into negative values due to type limitations that fs_ipg doesn't have. Clearly check_cgmagic() thinks the fs data is invalid yet somehow OpenBSD fsck thinks it works and the filesystem in general seems to work fine on both OS, at least from a basic usage standpoint (I haven't looked into anything more than basic I/O and whether fsck says clean or dirty). Is this check in check_cgmagic meant to compare internal representations or their signed/unsigned integer values? Is general compatibility between UFS in OpenBSD and FreeBSD purely accidental? If so, I'll drop the issue.
Given that your value of cgp->cg_old_niblk is > 2^15 says that at least under OpenBSD the declaration of cgp->cg_old_niblk as int16_t is wrong. Rather it should be declared as uint16_t. So it seems to me that the correct solution here is to change the declaration of cgp->cg_old_niblk to be uint16_t as it should. This change should not break any existing filesystems. As to the question of compatibility of OpenBSD's UFS to that of FreeBSD, we have managed to do that so far. So, I am in favor of keeping them compatible (at least for small changes such as this one).
(In reply to Kirk McKusick from comment #7) There are explicit mentions of 2^15-1 as the max valid values for ipg on UFS1 filesystems. Are they wrong ? That is the thing that needs to be audited before changing the type and check in fsck_ffs, otherwise fsck could start accepting corrupted filesystems.
(In reply to Konstantin Belousov from comment #8) This work was most likely already done by OpenBSD or, likely, NetBSD. Searching for their commit which modified the type is probably much less technically complicated than doing the audit anew.