Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message overstates how many sites were converted.
--
net: use ip4_parse() in receive validation
This patch converts br_validate_ipv4(), ip_rcv_core() and
nf_reject_iphdr_validate() to use ip4_parse(). They previously did an
open-coded pskb_may_pull() followed by version and ihl checks. The
returned thoff is then used in the later length checks.
> Convert the sites that pull and do version and ihl checks on the ipv4
> header and validate the packet transport offset to use ip4_parse.
>
> Sites that validate a read-only copy instead of pulling - the nft
> and nft_inner header_pointer validators, ip_check_defrag()'s
> skb_copy_bits, the flow dissector - stay open-coded for now.
[Severity: Low]
This isn't a bug, but is this description complete? A few sites still
call pskb_may_pull() and then open-code the version and ihl checks. The
read-only copy exclusion list above does not cover them.
nft_set_pktinfo_ipv4_ingress() in include/net/netfilter/nf_tables_ipv4.h:
if (!pskb_may_pull(pkt->skb, sizeof(*iph)))
return -1;
iph = ip_hdr(pkt->skb);
if (iph->ihl < 5 || iph->version != 4)
goto inhdr_error;
This has the same pattern as br_validate_ipv4(). The header_pointer
validator in that file is __nft_set_pktinfo_ipv4_validate(), not this
function.
ip_mc_check_iphdr() in net/ipv4/igmp.c:
if (!pskb_may_pull(skb, offset))
return -EINVAL;
iph = ip_hdr(skb);
if (iph->version != 4 || ip_hdrlen(skb) < sizeof(*iph))
return -EINVAL;
offset += ip_hdrlen(skb) - sizeof(*iph);
if (!pskb_may_pull(skb, offset))
return -EINVAL;
This looks like it maps directly onto ip4_parse(skb,
skb_network_offset(skb), &thoff).
nf_ct_bridge_pre() in net/bridge/netfilter/nf_conntrack_bridge.c calls
pskb_may_pull(skb, sizeof(struct iphdr)). nf_ct_br_ip_check() then
checks:
if (iph->ihl < 5 ||
iph->version != 4)
return -1;
All three sites are still open-coded at the end of the series, after
"net: account for ingress-resolved virtio net hdr state". At that point
the only ip4_parse() callers are br_validate_ipv4(), ip_rcv_core(),
nf_reject_iphdr_validate() and include/linux/virtio_net.h.
A smaller point: ip_rcv_core() still sets the transport header from ihl,
even though thoff is now in scope:
skb->transport_header = skb->network_header + iph->ihl*4;
This has no functional effect.
Should these sites be converted, or should the commit message list them
as staying open-coded?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791887415.git.mst%40redhat.com
