| Message ID | 20210618225502.170644-1-saeed@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] ethtool: strset: account for nesting in reply size | expand |
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 <mkubecek@suse.cz> 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 > --- > 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 >
On Sat, 2021-06-19 at 01:37 +0200, Michal Kubecek wrote: > 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: [...] > > 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 will go with the net fix, thanks for taking a look !
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++) {