Re: [PATCH net-next] ethtool: strset: account for nesting in reply size
From: Michal Kubecek <hidden>
Date: 2021-06-18 23:37:26
On Fri, Jun 18, 2021 at 03:55:02PM -0700, Saeed Mahameed wrote:
From: Saeed Mahameed <saeedm@nvidia.com> The cited patch revealed a bug in strset reply size where the calculation didn't include the 1st nla_nest_start(), a size of 4 Bytes in strset_fill_reply(). To fix the issue we account for the missing nla_nest 4Bytes by reporting them in strset_reply_size() Before this patch issuing "ethtool -k" command will produce the following call trace:
[...]
Fixes: 4d1fb7cde0cc ("ethtool: add a stricter length check")
Fixes: 7c87e32d2e38 ("ethtool: count header size in reply size estimate")
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Michal Kubecek <redacted>
Actually, the history was the other way around: Jakub first fixed this
bug discovered by sysbot with commit e175aef90269 ("ethtool: strset: fix
message length calculation") in net tree and it inspired him to refine
the length check to catch such issues more likely. Unfortunately the fix
hasn't been merged into net-next yet which is why you saw the warning.
At least we know for sure now that the new version of the check works
much better than the old one.
Note: I used nla_total_size(0); to report the missing bytes, i see in other places they use nla_total_size(sizeof(u32)). Since nla_nest uses a payload of 0, I prefer my version of nla_total_size(0); since it resembles what the nla_nest is actually doing. I might be wrong though :), comments ?
Out of the three fixes, personally I liked most the one which applied nla_total_len() to calculated length of the nest contents as it IMHO reflects the message structure best; but adding nla_total_size(0) also provides the same result so either does the trick. Michal
quoted hunk ↗ jump to hunk
--- net/ethtool/strset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c index b3029fff715d..23d517a61e08 100644 --- a/net/ethtool/strset.c +++ b/net/ethtool/strset.c@@ -349,8 +349,8 @@ static int strset_reply_size(const struct ethnl_req_info *req_base, { const struct strset_req_info *req_info = STRSET_REQINFO(req_base); const struct strset_reply_data *data = STRSET_REPDATA(reply_base); + int len = nla_total_size(0); /* account for nesting */ unsigned int i; - int len = 0; int ret; for (i = 0; i < ETH_SS_COUNT; i++) {-- 2.31.1