| Message ID | 20201005155753.2333882-1-kuba@kernel.org |
|---|---|
| Headers | show |
| Series | ethtool: allow dumping policies to user space | expand |
On Mon, 05 Oct 2020 20:56:29 +0200 Johannes Berg wrote: > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote: > > @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = { > > .start = ethnl_default_start, > > .dumpit = ethnl_default_dumpit, > > .done = ethnl_default_done, > > + .policy = ethnl_rings_get_policy, > > + .maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1, > > + > > }, > > If you find some other reason to respin, perhaps remove that blank line > :) > > Unrelated to that, it bothers me a bit that you put here the maxattr as > the ARRAY_SIZE(), which is of course fine, but then still have > > > @@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = { > > .max_attr = ETHTOOL_A_PRIVFLAGS_MAX, > > max_attr here, using the original define Ah, another good catch, this is obviously no longer needed. I will remove those members in v2. > yes, mostly the policies use > the define to size them, but they didn't really *need* to, and one might > make an argument that on the policy arrays the size might as well be > removed (and it be sized automatically based on the contents) since all > the unspecified attrs are rejected anyway. > > But with the difference it seems to me that it'd be possible to get this > mixed up? Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking was - use the format I like for the new code, but leave the existing rejects for a separate series / discussion. If we remove the rejects we still need something like extern struct nla_policy policy[lastattr + 1]; For array_size to work, but I think that's fine. And we'd get a compiler errors if the sizes don't match up. > I do see that you still need this to size the attrs for parsing them > even after patch 2 where this: > > > .req_info_size = sizeof(struct privflags_req_info), > > .reply_data_size = sizeof(struct privflags_reply_data), > > - .request_policy = privflags_get_policy, > > + .request_policy = ethnl_privflags_get_policy, > > gets removed completely. > > > Perhaps we can look up the genl_ops pointer, or add the ops pointer to > struct genl_info (could point to the temporary full struct that gets > populated, size of genl_info itself doesn't matter much since it's on > the stack and temporary), and then use ops->maxattr instead of > request_ops->max_attr in ethnl_default_parse()? Hm, maybe my split of patches 1 and 2 hurts more than it helps. Let me merge the two in v2.
On Mon, 2020-10-05 at 12:16 -0700, Jakub Kicinski wrote: > On Mon, 05 Oct 2020 20:56:29 +0200 Johannes Berg wrote: > > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote: > > > @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = { > > > .start = ethnl_default_start, > > > .dumpit = ethnl_default_dumpit, > > > .done = ethnl_default_done, > > > + .policy = ethnl_rings_get_policy, > > > + .maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1, > > > + > > > }, > > > > If you find some other reason to respin, perhaps remove that blank line > > :) > > > > Unrelated to that, it bothers me a bit that you put here the maxattr as > > the ARRAY_SIZE(), which is of course fine, but then still have > > > > > @@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = { > > > .max_attr = ETHTOOL_A_PRIVFLAGS_MAX, > > > > max_attr here, using the original define > > Ah, another good catch, this is obviously no longer needed. I will > remove those members in v2. Good point, I misread/misunderstood the code and thought it was still being used to size the parsing array, but that's of course no longer there since the genl core now does it. > > But with the difference it seems to me that it'd be possible to get this > > mixed up? > > Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking > was - use the format I like for the new code, but leave the existing > rejects for a separate series / discussion. > > If we remove the rejects we still need something like > > extern struct nla_policy policy[lastattr + 1]; Not sure I understand? You're using strict validation (I think), so attrs that are out of range will be rejected same as NLA_REJECT (well, with a different message) in __nla_validate_parse(): nla_for_each_attr(nla, head, len, rem) { u16 type = nla_type(nla); if (type == 0 || type > maxtype) { if (validate & NL_VALIDATE_MAXTYPE) { NL_SET_ERR_MSG_ATTR(extack, nla, "Unknown attribute type"); return -EINVAL; } In fact, if you're using strict validation even the default (0==NLA_UNSPEC) will be rejected, just like NLA_REJECT. Or am I confused somewhere? johannes
On Mon, 2020-10-05 at 12:25 -0700, Jakub Kicinski wrote: > On Mon, 05 Oct 2020 20:58:57 +0200 Johannes Berg wrote: > > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote: > > > @@ -47,19 +61,16 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info, > > > NL_SET_ERR_MSG(extack, "request header missing"); > > > return -EINVAL; > > > } > > > + /* Use most permissive header policy here, ops should specify their > > > + * actual header policy via NLA_POLICY_NESTED(), and the real > > > + * validation will happen in genetlink code. > > > + */ > > > ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, header, > > > - ethnl_header_policy, extack); > > > + ethnl_header_policy_stats, extack); > > > > Would it make sense to just remove the validation here? It's already > > done, so it just costs extra cycles and can't really fail, and if there > > are more diverse policies in the future this might also very quickly get > > out of hand? > > I was slightly worried I missed a command and need last line of defence, Ah. I was just about to suggest to put it into the family policy/maxattr but that won't work of course since this is nested. But actually what you _could_ put there is a dummy policy (non-NULL pointer) with a maxattr of 0, and then all attrs will be completely rejected for a command where the policy was missed. Not if you missed the NLA_POLICY_NESTED() link, though. johannes
On Mon, 05 Oct 2020 21:21:36 +0200 Johannes Berg wrote: > > > But with the difference it seems to me that it'd be possible to get this > > > mixed up? > > > > Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking > > was - use the format I like for the new code, but leave the existing > > rejects for a separate series / discussion. > > > > If we remove the rejects we still need something like > > > > extern struct nla_policy policy[lastattr + 1]; > > Not sure I understand? You're using strict validation (I think), so > attrs that are out of range will be rejected same as NLA_REJECT (well, > with a different message) in __nla_validate_parse(): > > nla_for_each_attr(nla, head, len, rem) { > u16 type = nla_type(nla); > > if (type == 0 || type > maxtype) { > if (validate & NL_VALIDATE_MAXTYPE) { > NL_SET_ERR_MSG_ATTR(extack, nla, > "Unknown attribute type"); > return -EINVAL; > } > > > In fact, if you're using strict validation even the default > (0==NLA_UNSPEC) will be rejected, just like NLA_REJECT. > > > Or am I confused somewhere? Yea, I think we're both confused. Agreed with the above. Are you suggesting: const struct nla_policy policy[/* no size */] = { [HEADER] = NLA_POLICY(...) [OTHER_ATTR] = NLA_POLICY(...) }; extern const struct nla_policy policy[/* no size */]; op = { .policy = policy, .max_attr = OTHER_ATTR, } ? What I'm saying is that my preference would be: const struct nla_policy policy[OTHER_ATTR + 1] = { [HEADER] = NLA_POLICY(...) [OTHER_ATTR] = NLA_POLICY(...) }; extern const struct nla_policy policy[OTHER_ATTR + 1]; op = { .policy = policy, .max_attr = ARRAY_SIZE(policy) - 1, } Since it's harder to forget to update the op (you don't have to update op, and compiler will complain about the extern out of sync).
On Mon, 2020-10-05 at 12:31 -0700, Jakub Kicinski wrote: > Yea, I think we're both confused. Agreed with the above. > > Are you suggesting: > > const struct nla_policy policy[/* no size */] = { > [HEADER] = NLA_POLICY(...) > [OTHER_ATTR] = NLA_POLICY(...) > }; > > extern const struct nla_policy policy[/* no size */]; > > op = { > .policy = policy, > .max_attr = OTHER_ATTR, > } No, that'd be awkward, for the reason you stated below. > What I'm saying is that my preference would be: > > const struct nla_policy policy[OTHER_ATTR + 1] = { > [HEADER] = NLA_POLICY(...) > [OTHER_ATTR] = NLA_POLICY(...) > }; > > extern const struct nla_policy policy[OTHER_ATTR + 1]; > > op = { > .policy = policy, > .max_attr = ARRAY_SIZE(policy) - 1, > } > > Since it's harder to forget to update the op (you don't have to update > op, and compiler will complain about the extern out of sync). Yeah. I was thinking the third way ;-) const struct nla_policy policy[] = { [HEADER] = NLA_POLICY(...) [OTHER_ATTR] = NLA_POLICY(...) }; op = { .policy = policy, .maxattr = ARRAY_SIZE(policy) - 1, }; Now you can freely add any attributes, and, due to strict validation, anything not specified in the policy will be rejected, whether by being out of range (> maxattr) or not specified (NLA_UNSPEC). johannes
On Mon, 05 Oct 2020 21:33:55 +0200 Johannes Berg wrote: > > What I'm saying is that my preference would be: > > > > const struct nla_policy policy[OTHER_ATTR + 1] = { > > [HEADER] = NLA_POLICY(...) > > [OTHER_ATTR] = NLA_POLICY(...) > > }; > > > > extern const struct nla_policy policy[OTHER_ATTR + 1]; > > > > op = { > > .policy = policy, > > .max_attr = ARRAY_SIZE(policy) - 1, > > } > > > > Since it's harder to forget to update the op (you don't have to update > > op, and compiler will complain about the extern out of sync). > > Yeah. > > I was thinking the third way ;-) > > const struct nla_policy policy[] = { > [HEADER] = NLA_POLICY(...) > [OTHER_ATTR] = NLA_POLICY(...) > }; > > op = { > .policy = policy, > .maxattr = ARRAY_SIZE(policy) - 1, > }; > > > Now you can freely add any attributes, and, due to strict validation, > anything not specified in the policy will be rejected, whether by being > out of range (> maxattr) or not specified (NLA_UNSPEC). 100%, but in ethtool policy is defined in a different compilation unit than the op array.
On Mon, 2020-10-05 at 12:41 -0700, Jakub Kicinski wrote: > > Now you can freely add any attributes, and, due to strict validation, > > anything not specified in the policy will be rejected, whether by being > > out of range (> maxattr) or not specified (NLA_UNSPEC). > > 100%, but in ethtool policy is defined in a different compilation unit > than the op array. Ah. OK, then that won't work, of course, never mind. I'd probably go with your preference then, but perhaps drop the actual size definition: const struct nla_policy policy[] = { ... }; extern const struct nla_policy policy[OTHER_ATTR + 1]; op = { .policy = policy, .max_attr = ARRAY_SIZE(policy) - 1, } But that'd really just be to save typing copying it if it ever changes, since it's compiler checked for consistency. johannes
On Mon, 05 Oct 2020 21:46:02 +0200 Johannes Berg wrote: > On Mon, 2020-10-05 at 12:41 -0700, Jakub Kicinski wrote: > > > > Now you can freely add any attributes, and, due to strict validation, > > > anything not specified in the policy will be rejected, whether by being > > > out of range (> maxattr) or not specified (NLA_UNSPEC). > > > > 100%, but in ethtool policy is defined in a different compilation unit > > than the op array. > > Ah. OK, then that won't work, of course, never mind. > > I'd probably go with your preference then, but perhaps drop the actual > size definition: > > const struct nla_policy policy[] = { > ... > }; > > extern const struct nla_policy policy[OTHER_ATTR + 1]; > > op = { > .policy = policy, > .max_attr = ARRAY_SIZE(policy) - 1, > } > > > But that'd really just be to save typing copying it if it ever changes, > since it's compiler checked for consistency. Sounds good, will do (unless Michal speaks up and prefers otherwise :)).
On 10/5/2020 12:31 PM, Jakub Kicinski wrote: > On Mon, 05 Oct 2020 21:21:36 +0200 Johannes Berg wrote: >>>> But with the difference it seems to me that it'd be possible to get this >>>> mixed up? >>> >>> Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking >>> was - use the format I like for the new code, but leave the existing >>> rejects for a separate series / discussion. >>> >>> If we remove the rejects we still need something like >>> >>> extern struct nla_policy policy[lastattr + 1]; >> >> Not sure I understand? You're using strict validation (I think), so >> attrs that are out of range will be rejected same as NLA_REJECT (well, >> with a different message) in __nla_validate_parse(): >> >> nla_for_each_attr(nla, head, len, rem) { >> u16 type = nla_type(nla); >> >> if (type == 0 || type > maxtype) { >> if (validate & NL_VALIDATE_MAXTYPE) { >> NL_SET_ERR_MSG_ATTR(extack, nla, >> "Unknown attribute type"); >> return -EINVAL; >> } >> >> >> In fact, if you're using strict validation even the default >> (0==NLA_UNSPEC) will be rejected, just like NLA_REJECT. >> >> >> Or am I confused somewhere? > > Yea, I think we're both confused. Agreed with the above. > > Are you suggesting: > > const struct nla_policy policy[/* no size */] = { > [HEADER] = NLA_POLICY(...) > [OTHER_ATTR] = NLA_POLICY(...) > }; > > extern const struct nla_policy policy[/* no size */]; > > op = { > .policy = policy, > .max_attr = OTHER_ATTR, > } > Why can't .max_attr here just be derived from ARRAY_SIZE? In this example, ARRAY_SIZE should return 2, so max_attr would be ARRAY_SIZE - 1. Well, I guess HEADER/OTHER_ATTR could make this a sparse array... in which case it might not work? > ? > > What I'm saying is that my preference would be: > > const struct nla_policy policy[OTHER_ATTR + 1] = { > [HEADER] = NLA_POLICY(...) > [OTHER_ATTR] = NLA_POLICY(...) > }; > > extern const struct nla_policy policy[OTHER_ATTR + 1]; > > op = { > .policy = policy, > .max_attr = ARRAY_SIZE(policy) - 1, > } > > Since it's harder to forget to update the op (you don't have to update > op, and compiler will complain about the extern out of sync). > Ahh.. Ok so I guess what you're doing here is defining the array size as OTHER_ATTR + 1, so that ARRAY_SIZE() - 1 guarantees to equal OTHER_ATTR.. so as long as OTHER_ATTR is the largest element in the array... But wouldn't that be the case in the previous example where array size is automatically determined... as long as you keep the index ordering in ascending order, then the size of the array would be 1 larger than the last element...
On 10/5/2020 12:33 PM, Johannes Berg wrote: > On Mon, 2020-10-05 at 12:31 -0700, Jakub Kicinski wrote: > >> Yea, I think we're both confused. Agreed with the above. >> >> Are you suggesting: >> >> const struct nla_policy policy[/* no size */] = { >> [HEADER] = NLA_POLICY(...) >> [OTHER_ATTR] = NLA_POLICY(...) >> }; >> >> extern const struct nla_policy policy[/* no size */]; >> >> op = { >> .policy = policy, >> .max_attr = OTHER_ATTR, >> } > > No, that'd be awkward, for the reason you stated below. > >> What I'm saying is that my preference would be: >> >> const struct nla_policy policy[OTHER_ATTR + 1] = { >> [HEADER] = NLA_POLICY(...) >> [OTHER_ATTR] = NLA_POLICY(...) >> }; >> >> extern const struct nla_policy policy[OTHER_ATTR + 1]; >> >> op = { >> .policy = policy, >> .max_attr = ARRAY_SIZE(policy) - 1, >> } >> >> Since it's harder to forget to update the op (you don't have to update >> op, and compiler will complain about the extern out of sync). > > Yeah. > > I was thinking the third way ;-) > > const struct nla_policy policy[] = { > [HEADER] = NLA_POLICY(...) > [OTHER_ATTR] = NLA_POLICY(...) > }; > > op = { > .policy = policy, > .maxattr = ARRAY_SIZE(policy) - 1, > }; > > > Now you can freely add any attributes, and, due to strict validation, > anything not specified in the policy will be rejected, whether by being > out of range (> maxattr) or not specified (NLA_UNSPEC). > > johannes > This is what I was thinking of as well.