| Message ID | 20241101214828.289752-3-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ena: two cleanups | expand |
Rosen Penev <rosenp@gmail.com> writes: > Use ethtool_sprintf to simplify the code. > > Because strings_buf is separate from buf, it needs to be > incremented > separately. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > drivers/net/ethernet/amazon/ena/ena_ethtool.c | 17 > ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c > b/drivers/net/ethernet/amazon/ena/ena_ethtool.c > index fa9d7b8ec00d..96fa55a88faf 100644 > --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c > +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c > @@ -1120,7 +1120,7 @@ static void ena_dump_stats_ex(struct > ena_adapter *adapter, u8 *buf) > u8 *strings_buf; > u64 *data_buf; > int strings_num; > - int i, rc; > + int i; > > strings_num = ena_get_sw_stats_count(adapter); > if (strings_num <= 0) { > @@ -1149,17 +1149,16 @@ static void ena_dump_stats_ex(struct > ena_adapter *adapter, u8 *buf) > /* If there is a buffer, dump stats, otherwise print them > to dmesg */ > if (buf) > for (i = 0; i < strings_num; i++) { > - rc = snprintf(buf, ETH_GSTRING_LEN + > sizeof(u64), > - "%s %llu\n", > - strings_buf + i * > ETH_GSTRING_LEN, > - data_buf[i]); > - buf += rc; > + ethtool_sprintf(&buf, "%s %llu\n", > strings_buf, > + data_buf[i]); > + strings_buf += ETH_GSTRING_LEN; > } > else > - for (i = 0; i < strings_num; i++) > + for (i = 0; i < strings_num; i++) { > netif_err(adapter, drv, netdev, "%s: > %llu\n", > - strings_buf + i * > ETH_GSTRING_LEN, > - data_buf[i]); > + strings_buf, data_buf[i]); > + strings_buf += ETH_GSTRING_LEN; > + } > > kfree(strings_buf); > kfree(data_buf); Thank you for submitting the patch. If I'm not mistaken, there are some bugs introduced here: 1. You update string_buf pointer itself, but later you pass the variable to kfree, this would likely end up freeing the wrong address (/causing a kernel panic) 2. The ethtool_sprintf increases the `buf` pointer by ETH_GSTRING_LEN, while the previous code increased it by (ETH_GSTRING_LEN + sizeof(u64)) bytes. This causes a corruption in the buffer. This function and mechanism is already planned for an overhaul in a future patch. We prefer to leave this patch out. Thanks, Shay
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c index fa9d7b8ec00d..96fa55a88faf 100644 --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c @@ -1120,7 +1120,7 @@ static void ena_dump_stats_ex(struct ena_adapter *adapter, u8 *buf) u8 *strings_buf; u64 *data_buf; int strings_num; - int i, rc; + int i; strings_num = ena_get_sw_stats_count(adapter); if (strings_num <= 0) { @@ -1149,17 +1149,16 @@ static void ena_dump_stats_ex(struct ena_adapter *adapter, u8 *buf) /* If there is a buffer, dump stats, otherwise print them to dmesg */ if (buf) for (i = 0; i < strings_num; i++) { - rc = snprintf(buf, ETH_GSTRING_LEN + sizeof(u64), - "%s %llu\n", - strings_buf + i * ETH_GSTRING_LEN, - data_buf[i]); - buf += rc; + ethtool_sprintf(&buf, "%s %llu\n", strings_buf, + data_buf[i]); + strings_buf += ETH_GSTRING_LEN; } else - for (i = 0; i < strings_num; i++) + for (i = 0; i < strings_num; i++) { netif_err(adapter, drv, netdev, "%s: %llu\n", - strings_buf + i * ETH_GSTRING_LEN, - data_buf[i]); + strings_buf, data_buf[i]); + strings_buf += ETH_GSTRING_LEN; + } kfree(strings_buf); kfree(data_buf);
Use ethtool_sprintf to simplify the code. Because strings_buf is separate from buf, it needs to be incremented separately. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- drivers/net/ethernet/amazon/ena/ena_ethtool.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)