geom_part: Check number of GPT entries and size of GPT entry

[zlei]

Summary

Current specification does not have upper limit of the number of partition entries and the size of partition entry. In 799eac8c3df597179bbb3b078362f3ff03993a1a Andrey V. Elsukov introduced a limit maximum number of GPT entries to 4k, but that is for write routine (gpart create) only. When attaching disks that have large number of GPT entries exceeding the limit, or disks with large size of partition entry, it is still possible to exhaust kernel memory.

1. Reuse the limit of the maximum number of partition entries.
2. Limit the maximum size of GPT entry to 1k.

In current specification (2.10) the size of GPT entry is 128 * 2^n while n >= 0, and the size - 128 is reserved. 1k should be sufficient enough for foreseen future.

PR: 266548

Test Plan

Attach a specially crafted disk image with gpart(modified kernel to support creating large GPT entry numbers):

root@:~ # truncate -s 64M test.img
root@:~ # mdconfig test.img
md0
root@:~ # gpart create -s GPT -n 8192 md0
md0 created
root@:~ # mdconfig -du 0

Reattach the disk image.

root@:~ # mdconfig test.img
GEOM: md0: unsupported GPT detected.
GEOM: md0: number of GPT entries: 8192, entry size: 128B.
GEOM: md0: maximum supported number of GPT entries: 4096, entry size: 1024B.
GEOM: md0: GPT rejected.

[zlei]

sys/geom/part/g_part_gpt.c

153

May it deserve a tunable ?

[imp]

sys/geom/part/g_part_gpt.c

526

If you made it a tunable, then you'd use that size here, but keep the gps_maxent as above.
If you don't make it a tunable, then maybe you should use g_part_gpt_scheme.gps_maxent here.

Even though I suggested a tunable, I'm now less sure that it is a good idea than when I suggested it.

974

Other than a better error message, what purpose does this code serve? The following catch-all would already capture this (g_free(NULL) is a nop).

[markj]

Thanks for working on this.

sys/geom/part/g_part_gpt.c

526

I think this check should come before the multiplication. Then we don't have to worry about overflow.

I don't have a strong feeling about having a tunable. We can always add one after the fact.

974

I agree. It is probably worth reporting the problem with printf(), but 1) we should say _why_ the GPT is unsupported, 2) we should do it when reading the table, I think.

[zlei]

Read GPT table fully seems too heavy to report unsupported accurately. We try our best here.
I'm still struggling as it shall be rare that both primary and backup table headers corrupt at the same time and have good CRCs and "equals" to each other.

[zlei]

sys/geom/part/g_part_gpt.c

526

I think this check should come before the multiplication. Then we don't have to worry about overflow.

If the table size check fail, then definitely the header is invalid. We must not report invalid as unsupported.

Let me clarify, the unsupported means the header looks valid but not supported (due to implementation)

974

I agree. It is probably worth reporting the problem with printf(), but 1) we should say _why_ the GPT is unsupported,

I'm still struggling with that. Should we report maximum supported header entry count and size ?

2. we should do it when reading the table, I think.

Too heavy to read large table.

[zlei]

2. we should do it when reading the table, I think.

Or maybe we read at most say 1M table and report unsupported when header table size exceed.
This should be precisely and not that heavy.

[zlei]

I misread @markj 's comment:

2. we should do it when reading the table, I think.

The GPT entry header is sanity although the entry number or the entry size is not supported.
So mark GPT header as NOT_SUPPORTED does not make sense. Move the check to gpt_read_tbl().

Also add detailed error message to explain why not supported

[zlei]

Ping @imp @markj .

[markj]

Looks ok to me, thanks. Just some minor notes about the messages printed to the console.

sys/geom/part/g_part_gpt.c

977

979

These two messages would be clearer to me if you replaced "number" with "count". Or write "number of GPT entries" instead of "GPT entry number".

979

[zlei]

Update as @markj suggested.

[zlei]

Oops, add missing unit B(ytes)

[zlei]

sys/geom/part/g_part_gpt.c

974

Other than a better error message, what purpose does this code serve? The following catch-all would already capture this (g_free(NULL) is a nop).

It deserves a good error message.
I've planned to emit some NULL checks below.

[markj]

sys/geom/part/g_part_gpt.c

977

979

[zlei]

Address @markj 's comment

[zlei]

sys/geom/part/g_part_gpt.c

977

My bad ...

[zlei]

@imp @markj Thanks for the review !