| Message ID | 20230418234354.582693-4-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: act_pedit: minor improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 18 |
| netdev/checkpatch | warning | WARNING: line length of 90 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote: > @@ -414,12 +420,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, > sizeof(_d), &_d); > if (!d) > goto bad; > - offset += (*d & tkey->offmask) >> tkey->shift; > - } > > - if (offset % 4) { > - pr_info("tc action pedit offset must be on 32 bit boundaries\n"); > - goto bad; > + offset += (*d & tkey->offmask) >> tkey->shift; this line loads part of the offset from packet data, so it's not exactly equivalent to the init time check. It's unlikely to be used but I think that rejecting cur % 4 vs data patch check only is technically a functional change, so needs to be discussed in the commit msg. > + if (offset % 4) { > + pr_info("tc action pedit offset must be on 32 bit boundaries\n"); > + goto bad; > + }
On 20/04/2023 23:41, Jakub Kicinski wrote: > On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote: >> @@ -414,12 +420,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, >> sizeof(_d), &_d); >> if (!d) >> goto bad; >> - offset += (*d & tkey->offmask) >> tkey->shift; >> - } >> >> - if (offset % 4) { >> - pr_info("tc action pedit offset must be on 32 bit boundaries\n"); >> - goto bad; >> + offset += (*d & tkey->offmask) >> tkey->shift; > > this line loads part of the offset from packet data, so it's not > exactly equivalent to the init time check. The code uses 'tkey->offmask' as a check for static offsets vs packet derived offsets, which have different handling. By checking the static offsets at init we can move the datapath 'offset % 4' check for the packet derived offsets only. Note that this change only affects the offsets defined in 'tkey->off', the 'at' offset logic stays the same. My intention was to keep the code semantically the same. Did I miss anything? > It's unlikely to be used > but I think that rejecting cur % 4 vs data patch check only is > technically a functional change, so needs to be discussed in the commit > msg. >
On Fri, 21 Apr 2023 12:12:54 -0300 Pedro Tammela wrote: > On 20/04/2023 23:41, Jakub Kicinski wrote: > > On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote: > >> @@ -414,12 +420,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, > >> sizeof(_d), &_d); > >> if (!d) > >> goto bad; > >> - offset += (*d & tkey->offmask) >> tkey->shift; > >> - } > >> > >> - if (offset % 4) { > >> - pr_info("tc action pedit offset must be on 32 bit boundaries\n"); > >> - goto bad; > >> + offset += (*d & tkey->offmask) >> tkey->shift; > > > > this line loads part of the offset from packet data, so it's not > > exactly equivalent to the init time check. > > The code uses 'tkey->offmask' as a check for static offsets vs packet > derived offsets, which have different handling. > By checking the static offsets at init we can move the datapath > 'offset % 4' check for the packet derived offsets only. > > Note that this change only affects the offsets defined in 'tkey->off', > the 'at' offset logic stays the same. > My intention was to keep the code semantically the same. > Did I miss anything? You are now erroring out if the static offset is not divisible by 4, and technically it was possible to construct a case where that'd work previously - if static offset was 2 and we load another 2 from the packet, no? If so it needs to be mentioned in the commit msg.
On 21/04/2023 12:15, Jakub Kicinski wrote: > On Fri, 21 Apr 2023 12:12:54 -0300 Pedro Tammela wrote: >> On 20/04/2023 23:41, Jakub Kicinski wrote: >>> On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote: >>>> @@ -414,12 +420,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, >>>> sizeof(_d), &_d); >>>> if (!d) >>>> goto bad; >>>> - offset += (*d & tkey->offmask) >> tkey->shift; >>>> - } >>>> >>>> - if (offset % 4) { >>>> - pr_info("tc action pedit offset must be on 32 bit boundaries\n"); >>>> - goto bad; >>>> + offset += (*d & tkey->offmask) >> tkey->shift; >>> >>> this line loads part of the offset from packet data, so it's not >>> exactly equivalent to the init time check. >> >> The code uses 'tkey->offmask' as a check for static offsets vs packet >> derived offsets, which have different handling. >> By checking the static offsets at init we can move the datapath >> 'offset % 4' check for the packet derived offsets only. >> >> Note that this change only affects the offsets defined in 'tkey->off', >> the 'at' offset logic stays the same. >> My intention was to keep the code semantically the same. >> Did I miss anything? > > You are now erroring out if the static offset is not divisible by 4, > and technically it was possible to construct a case where that'd work > previously - if static offset was 2 and we load another 2 from the > packet, no? > True! It seems though that the iproute2 packing broke this feature: tc action add action pedit ex munge offset 2 u16 at 128 ff 0 clear offset will be rounded down to 0 in iproute2, so if in the datapath at 128 sums 2, it will fail the offset check since 2 % 4 != 0. Let me check with Jamal what he thinks about this change since netlink could do it still. This slipped under our radar, thanks for catching it! > If so it needs to be mentioned in the commit msg.
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index c8f27a384800..ef6cdf17743b 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -256,6 +256,12 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla, for (i = 0; i < nparms->tcfp_nkeys; ++i) { u32 cur = nparms->tcfp_keys[i].off; + if (cur % 4) { + NL_SET_ERR_MSG_MOD(extack, "Offsets must be on 32bit boundaries"); + ret = -EINVAL; + goto put_chain; + } + /* sanitize the shift value for any later use */ nparms->tcfp_keys[i].shift = min_t(size_t, BITS_PER_TYPE(int) - 1, @@ -414,12 +420,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, sizeof(_d), &_d); if (!d) goto bad; - offset += (*d & tkey->offmask) >> tkey->shift; - } - if (offset % 4) { - pr_info("tc action pedit offset must be on 32 bit boundaries\n"); - goto bad; + offset += (*d & tkey->offmask) >> tkey->shift; + if (offset % 4) { + pr_info("tc action pedit offset must be on 32 bit boundaries\n"); + goto bad; + } } if (!offset_valid(skb, hoffset + offset)) {