Page MenuHomeFreeBSD

[geom] g_label_ufs may not find UFS1 header in case of usage GEOM_UZIP
Needs ReviewPublic

Authored by mizhka on May 4 2016, 6:49 PM.

Details

Summary

During lookup of UFS magics g_label_ufs makes 2 checks:

  • magic
  • filesystem size is equal to size of geom

But if image is built by mkuzip, geom size can be higher due to sector alignment: mkuzip uses large sectors (default is 16Kb) when fs has 512-byte sectors.

This patch allows difference between FS and geom sizes, but no more than 256 FS sectors.

Test Plan

Tested on UFS1 filesystem with small size on Asus RT-N53 router.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mizhka updated this revision to Diff 15900.May 4 2016, 6:49 PM
mizhka retitled this revision from to [geom] g_label_ufs may not find UFS1 header in case of usage GEOM_UZIP.
mizhka updated this object.
mizhka edited the test plan for this revision. (Show Details)
mizhka added a reviewer: sobomax.
mizhka set the repository for this revision to rS FreeBSD src repository.
sobomax accepted this revision.May 4 2016, 10:16 PM
sobomax edited edge metadata.

Looks good to me, I've noticed that thing too in other contexts, so it's does not affect uzip only. GEOM_LABEL appears much more restrictive validating than UFS itself, which would attach to that FS just fine without a warning or anything. In general, on one hand it would be nice to get geom_label use the same method for validating superblock as UFS itself would, instead of re-invent the wheel with G_LABEL_MAX_DIFF etc. On the other hand, since it's auto-discovering it probably needs a bigger barrier to filter out possible false positives.

sys/geom/label/g_label_ufs.c
48 ↗(On Diff #15900)

My first impression was that is somewhat over-engineered. It's like adding "#define sum(a, b) (a + b)" to do 1 + 2.

But I can see some merit of it, however I'd extend it to take pp and fs:

G_LABEL_DIFF(pp, fs, sm) (((pp)->(mediasize) / (fs)->(fs_fsize)) - (fs)->(sm))

Then your code would reduce from:

G_LABEL_DIFF(pp->mediasize / fs->fs_fsize, fs->fs_old_size) -> G_LABEL_DIFF(pp, fs, fs_old_size)

You can also further reduce it by combining it with the comparison, i.e.:

G_LABEL_CMP(pp, fs, sm) ((((pp)->(mediasize) / (fs)->(fs_fsize)) - (fs)->(sm)) < G_LABEL_MAX_DIFF), then the whole comparison becomes:

if (fs->fs_magic == FS_UFS1_MAGIC && fs->fs_fsize > 0 && (G_LABEL_CMP(pp, fs, fs_old_size) || (G_LABEL_CMP(pp, fs, fs_providersize))

This revision is now accepted and ready to land.May 4 2016, 10:16 PM

P.S. Please use git diff -U 9999 for making patch for posting here. It gives more context and makes review simpler.

mizhka updated this revision to Diff 15916.May 5 2016, 7:52 AM
mizhka edited edge metadata.

improved code style :)

This revision now requires review to proceed.May 5 2016, 7:52 AM
mizhka marked an inline comment as done.May 5 2016, 7:53 AM
sobomax accepted this revision.May 6 2016, 7:30 PM
sobomax edited edge metadata.

Looks good, some minor suggestions. Also make sure it wraps up at 80 column mark.

sys/geom/label/g_label_ufs.c
52 ↗(On Diff #15916)

Not strictly required, but it's good practice to put names of "variables" in the cpp macro into (), otherwise say you have a macro like this:

#define mul(a, b) (a * b)

Then you can use it in your code in a way as you would any function, i.e. mul(2, 10 + 1) and expect it to evaluate to "22" (i.e. 2 * 11). However it would expand into 2 * 10 + 1 and hence evaluate to "21". Would work properly if you do it:

#define mul(a, b) ((a) * (b))

This is especially easy to screw if you accept pointers as "arguments", where you may need to do pointer arithmetic when calling macro, i.e. MACRO(p + i, x, y), would evaluate into "p + i->foobar" unless you have () around (p).

This revision is now accepted and ready to land.May 6 2016, 7:30 PM
mizhka added inline comments.May 6 2016, 8:02 PM
sys/geom/label/g_label_ufs.c
52 ↗(On Diff #15916)

Thanks! I've removed brackets due to compiler errors. Issue might be in one bracket, but I removed all. I'll check tomorrow

mizhka updated this revision to Diff 16129.May 10 2016, 12:47 PM
mizhka edited edge metadata.
  • fixed style(9) issues
  • fixed style of macro
This revision now requires review to proceed.May 10 2016, 12:47 PM
mizhka marked an inline comment as done.May 10 2016, 12:48 PM
This revision was automatically updated to reflect the committed changes.
sobomax reopened this revision.Jul 18 2016, 8:38 PM

@butcher says some improvements might be needed:

  • any particular reason to use abs() in there? We specifically want to allow padding, not truncation.
  • I don't quite understand that complain but he says "2. fs_fsize is size of frag blocks, so use it as divider looks incorrect", but let me post it here just for the record. Could be some pre-existing bug, as no math is changed, just condition relaxed.
sobomax resigned from this revision.Mar 26 2019, 5:43 PM

Not sure why this is still open. @mizhka_gmail.com please GC?