| Message ID | 20241202-packing-pack-fields-and-ice-implementation-v7-2-ed22e38e6c65@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | lib: packing: introduce and use (un)pack_fields | expand |
On Mon, Dec 02, 2024 at 04:26:25PM -0800, Jacob Keller wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Most of the sanity checks in pack() and unpack() can be covered at > compile time. There is only one exception, and that is truncation of the > uval during a pack() operation. > > We'd like the error-less __pack() to catch that condition as well. But > at the same time, it is currently the responsibility of consumer drivers > (currently just sja1105) to print anything at all when this error > occurs, and then discard the return code. > > We can just print a loud warning in the library code and continue with > the truncated __pack() operation. In practice, having the warning is > very important, see commit 24deec6b9e4a ("net: dsa: sja1105: disallow > C45 transactions on the BASE-TX MDIO bus") where the bug was caught > exactly by noticing this print. > > Add the first print to the packing library, and at the same time remove > the print for the same condition from the sja1105 driver, to avoid > double printing. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- Somehow this is missing your sign off.
On 12/3/2024 4:43 AM, Vladimir Oltean wrote: > On Mon, Dec 02, 2024 at 04:26:25PM -0800, Jacob Keller wrote: >> From: Vladimir Oltean <vladimir.oltean@nxp.com> >> >> Most of the sanity checks in pack() and unpack() can be covered at >> compile time. There is only one exception, and that is truncation of the >> uval during a pack() operation. >> >> We'd like the error-less __pack() to catch that condition as well. But >> at the same time, it is currently the responsibility of consumer drivers >> (currently just sja1105) to print anything at all when this error >> occurs, and then discard the return code. >> >> We can just print a loud warning in the library code and continue with >> the truncated __pack() operation. In practice, having the warning is >> very important, see commit 24deec6b9e4a ("net: dsa: sja1105: disallow >> C45 transactions on the BASE-TX MDIO bus") where the bug was caught >> exactly by noticing this print. >> >> Add the first print to the packing library, and at the same time remove >> the print for the same condition from the sja1105 driver, to avoid >> double printing. >> >> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >> --- > > Somehow this is missing your sign off. Oops.
diff --git a/drivers/net/dsa/sja1105/sja1105_static_config.c b/drivers/net/dsa/sja1105/sja1105_static_config.c index baba204ad62f6b507a6ccf3337248dd02b777249..3d790f8c6f4dab3640ede014345ef469fefb7085 100644 --- a/drivers/net/dsa/sja1105/sja1105_static_config.c +++ b/drivers/net/dsa/sja1105/sja1105_static_config.c @@ -26,12 +26,8 @@ void sja1105_pack(void *buf, const u64 *val, int start, int end, size_t len) pr_err("Start bit (%d) expected to be larger than end (%d)\n", start, end); } else if (rc == -ERANGE) { - if ((start - end + 1) > 64) - pr_err("Field %d-%d too large for 64 bits!\n", - start, end); - else - pr_err("Cannot store %llx inside bits %d-%d (would truncate)\n", - *val, start, end); + pr_err("Field %d-%d too large for 64 bits!\n", + start, end); } dump_stack(); } diff --git a/lib/packing.c b/lib/packing.c index f237b8af99f5fa8e839c38126769c50b2bfe6361..09a2d195b9433b61c86f3b63ff019ab319c83e97 100644 --- a/lib/packing.c +++ b/lib/packing.c @@ -59,8 +59,17 @@ static void __pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, */ int plogical_first_u8 = startbit / BITS_PER_BYTE; int plogical_last_u8 = endbit / BITS_PER_BYTE; + int value_width = startbit - endbit + 1; int box; + /* Check if "uval" fits in "value_width" bits. + * The test only works for value_width < 64, but in the latter case, + * any 64-bit uval will surely fit. + */ + WARN(value_width < 64 && uval >= (1ull << value_width), + "Cannot store 0x%llx inside bits %zu-%zu - will truncate\n", + uval, startbit, endbit); + /* Iterate through an idealistic view of the pbuf as an u64 with * no quirks, u8 by u8 (aligned at u8 boundaries), from high to low * logical bit significance. "box" denotes the current logical u8. @@ -143,9 +152,6 @@ static void __pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen, u8 quirks) { - /* width of the field to access in the pbuf */ - u64 value_width; - /* startbit is expected to be larger than endbit, and both are * expected to be within the logically addressable range of the buffer. */ @@ -153,19 +159,7 @@ int pack(void *pbuf, u64 uval, size_t startbit, size_t endbit, size_t pbuflen, /* Invalid function call */ return -EINVAL; - value_width = startbit - endbit + 1; - if (unlikely(value_width > 64)) - return -ERANGE; - - /* Check if "uval" fits in "value_width" bits. - * If value_width is 64, the check will fail, but any - * 64-bit uval will surely fit. - */ - if (value_width < 64 && uval >= (1ull << value_width)) - /* Cannot store "uval" inside "value_width" bits. - * Truncating "uval" is most certainly not desirable, - * so simply erroring out is appropriate. - */ + if (unlikely(startbit - endbit >= 64)) return -ERANGE; __pack(pbuf, uval, startbit, endbit, pbuflen, quirks);