| Message ID | 20231010110828.200709-2-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: finish conversion to generated split_ops | expand |
On Tue, 2023-10-10 at 13:08 +0200, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Currently, split ops of doit and dumpit are merged into a single iter > item when they are subsequent. However, there is no guarantee that the > dumpit op is for the same cmd as doit op. > > Fix this by checking if cmd is the same for both. It's confusing, but I don't think this is needed, I believe genl_validate_ops() ensures that do comes before dump, and the commands are sorted, so that you cannot end up in this situation? And even if you can end up in this situation, I don't think this patch is the correct way to address it - we should then improve the validation in genl_validate_ops() instead. (And maybe add a comment here, but ...) johannes
Tue, Oct 10, 2023 at 01:24:31PM CEST, johannes@sipsolutions.net wrote: >On Tue, 2023-10-10 at 13:08 +0200, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> Currently, split ops of doit and dumpit are merged into a single iter >> item when they are subsequent. However, there is no guarantee that the >> dumpit op is for the same cmd as doit op. >> >> Fix this by checking if cmd is the same for both. > >It's confusing, but I don't think this is needed, I believe >genl_validate_ops() ensures that do comes before dump, and the commands >are sorted, so that you cannot end up in this situation? Apply this patchset w/o this patch and you'll hit it :) Otherwise I would not care... The problem is dumpit op of cmd Y with previous doit op of cmd X. They should not be merged, they are not same cmd, yet existing code does that. > >And even if you can end up in this situation, I don't think this patch >is the correct way to address it - we should then improve the validation >in genl_validate_ops() instead. The problem is incorrect merge of 2 consequent split ops into iter. This function does the merging. Where else to fix it? > >(And maybe add a comment here, but ...) > >johannes >
On Tue, 2023-10-10 at 13:39 +0200, Jiri Pirko wrote: > Apply this patchset w/o this patch and you'll hit it :) Otherwise I > would not care... :) > The problem is dumpit op of cmd Y with previous doit op of cmd X. They > should not be merged, they are not same cmd, yet existing code does > that. Yeah, on second thought, you're right, if you have CMDs X < Y and then X/do Y/dump (and no X/dump, nor Y/do) then indeed this can happen. Sorry for the noise. johannes
On Tue, 2023-10-10 at 13:08 +0200, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Currently, split ops of doit and dumpit are merged into a single iter > item when they are subsequent. However, there is no guarantee that the > dumpit op is for the same cmd as doit op. > > Fix this by checking if cmd is the same for both. > > Fixes: b8fd60c36a44 ("genetlink: allow families to use split ops directly") > Signed-off-by: Jiri Pirko <jiri@nvidia.com> Reviewed-by: Johannes Berg <johannes@sipsolutions.net> johannes
On Tue, 10 Oct 2023 13:08:20 +0200 Jiri Pirko wrote: > Fixes: b8fd60c36a44 ("genetlink: allow families to use split ops directly") Drop Fixes, add "currently no family declares ops which could trigger this issue". > if (i + cnt < family->n_split_ops && > - family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP) { > + family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP && > + (!cnt || > + (cnt && family->split_ops[i + cnt].cmd == iter->doit.cmd))) { Why are you checking cnt? if do was not found cmd will be 0, which cannot mis-match.
Tue, Oct 10, 2023 at 08:48:45PM CEST, kuba@kernel.org wrote: >On Tue, 10 Oct 2023 13:08:20 +0200 Jiri Pirko wrote: >> Fixes: b8fd60c36a44 ("genetlink: allow families to use split ops directly") > >Drop Fixes, add "currently no family declares ops which could trigger >this issue". Yeah, we need fixes semantics written down somewhere. I can do it, sure. > >> if (i + cnt < family->n_split_ops && >> - family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP) { >> + family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP && >> + (!cnt || >> + (cnt && family->split_ops[i + cnt].cmd == iter->doit.cmd))) { > >Why are you checking cnt? if do was not found cmd will be 0, which >cannot mis-match. Correct. Will remove cnt check.
Wed, Oct 11, 2023 at 08:08:57AM CEST, jiri@resnulli.us wrote: >Tue, Oct 10, 2023 at 08:48:45PM CEST, kuba@kernel.org wrote: >>On Tue, 10 Oct 2023 13:08:20 +0200 Jiri Pirko wrote: >>> Fixes: b8fd60c36a44 ("genetlink: allow families to use split ops directly") >> >>Drop Fixes, add "currently no family declares ops which could trigger >>this issue". > >Yeah, we need fixes semantics written down somewhere. >I can do it, sure. I found 2 mentions that relate to netdev regarging Fixes: Quoting Documentation/process/submitting-patches.rst: If your patch fixes a bug in a specific commit, e.g. you found an issue using ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of the SHA-1 ID, and the one line summary. Quoting Documentation/process/maintainer-netdev.rst: - for fixes the ``Fixes:`` tag is required, regardless of the tree This patch fixes a bug, sure, bug is not hit by existing code, but still it is present. Why it is wrong to put "Fixes" in this case? Could you please document this? > > >> >>> if (i + cnt < family->n_split_ops && >>> - family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP) { >>> + family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP && >>> + (!cnt || >>> + (cnt && family->split_ops[i + cnt].cmd == iter->doit.cmd))) { >> >>Why are you checking cnt? if do was not found cmd will be 0, which >>cannot mis-match. > >Correct. Will remove cnt check.
On Wed, 11 Oct 2023 13:27:05 +0200 Jiri Pirko wrote: > >Yeah, we need fixes semantics written down somewhere. > >I can do it, sure. > > I found 2 mentions that relate to netdev regarging Fixes: > > Quoting Documentation/process/submitting-patches.rst: > If your patch fixes a bug in a specific commit, e.g. you found an issue using > ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of > the SHA-1 ID, and the one line summary. > > Quoting Documentation/process/maintainer-netdev.rst: > - for fixes the ``Fixes:`` tag is required, regardless of the tree > > This patch fixes a bug, sure, bug is not hit by existing code, but still > it is present. > > Why it is wrong to put "Fixes" in this case? > Could you please document this? I think you're asking me to document what a bug is because the existing doc clearly says Fixes is for bugs. If the code does not misbehave, there is no bug.
Wed, Oct 11, 2023 at 06:47:02PM CEST, kuba@kernel.org wrote: >On Wed, 11 Oct 2023 13:27:05 +0200 Jiri Pirko wrote: >> >Yeah, we need fixes semantics written down somewhere. >> >I can do it, sure. >> >> I found 2 mentions that relate to netdev regarging Fixes: >> >> Quoting Documentation/process/submitting-patches.rst: >> If your patch fixes a bug in a specific commit, e.g. you found an issue using >> ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of >> the SHA-1 ID, and the one line summary. >> >> Quoting Documentation/process/maintainer-netdev.rst: >> - for fixes the ``Fixes:`` tag is required, regardless of the tree >> >> This patch fixes a bug, sure, bug is not hit by existing code, but still >> it is present. >> >> Why it is wrong to put "Fixes" in this case? >> Could you please document this? > >I think you're asking me to document what a bug is because the existing >doc clearly says Fixes is for bugs. If the code does not misbehave, >there is no bug. Interesting. Will try to remember :)
On 10/11/2023 9:47 AM, Jakub Kicinski wrote: > On Wed, 11 Oct 2023 13:27:05 +0200 Jiri Pirko wrote: >>> Yeah, we need fixes semantics written down somewhere. >>> I can do it, sure. >> >> I found 2 mentions that relate to netdev regarging Fixes: >> >> Quoting Documentation/process/submitting-patches.rst: >> If your patch fixes a bug in a specific commit, e.g. you found an issue using >> ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of >> the SHA-1 ID, and the one line summary. >> >> Quoting Documentation/process/maintainer-netdev.rst: >> - for fixes the ``Fixes:`` tag is required, regardless of the tree >> >> This patch fixes a bug, sure, bug is not hit by existing code, but still >> it is present. >> >> Why it is wrong to put "Fixes" in this case? >> Could you please document this? > > I think you're asking me to document what a bug is because the existing > doc clearly says Fixes is for bugs. If the code does not misbehave, > there is no bug. > Well this code misbehaves if given the right input. We just don't give it that input today. I would have called that a bug too. But from a strict sense of "can you make this fail on a current kernel" the answer is no, since no families exist which have this requirement until after this series.
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 8315d31b53db..34346a73a0d6 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -225,7 +225,9 @@ static void genl_op_from_split(struct genl_op_iter *iter) } if (i + cnt < family->n_split_ops && - family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP) { + family->split_ops[i + cnt].flags & GENL_CMD_CAP_DUMP && + (!cnt || + (cnt && family->split_ops[i + cnt].cmd == iter->doit.cmd))) { iter->dumpit = family->split_ops[i + cnt]; genl_op_fill_in_reject_policy_split(family, &iter->dumpit); cnt++;