Re: [PATCH net-next v2 3/8] gre: make gre_parse_header() report a drop reason
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-14 04:37:21
Also in:
lkml
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] The new interface comment added above gre_parse_header() (net/ipv4/gre_demux.c:65-68) states that a NULL @reason means "the… -- gre: make gre_parse_header() report a drop reason This replaces the unused csum_err output parameter of gre_parse_header() with an enum skb_drop_reason pointer, adds SKB_DROP_REASON_GRE_INVALID_HDR and SKB_DROP_REASON_GRE_CSUM, and converts the ip_gre, ip6_gre and gre_demux receive paths to kfree_skb_reason(). No Link: tags were present in the commit header. One question below about the documented meaning of a NULL reason.
A NULL reason keeps the meaning a NULL csum_err had: the caller is not interested in it and the checksum must not be verified. This matters for the ICMP error handlers, which only get a part of the original packet and must not drop it when the checksum does not validate.
[Severity: Low] Is the phrase "the checksum must not be verified" accurate here? With a NULL reason the checksum is still computed, and on the success path the skb checksum state is still modified; only the reporting of a failure is skipped. Would "a checksum failure must not be reported" describe what the code does? The same wording is added to the kerneldoc-style comment below, see the inline note.
quoted hunk ↗ jump to hunk
diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c index 96fd7dc6d82dd..efd5f60a9c591 100644 --- a/net/ipv4/gre_demux.c +++ b/net/ipv4/gre_demux.c@@ -58,26 +58,43 @@ EXPORT_SYMBOL_GPL(gre_del_protocol); /* Fills in tpi and returns header length to be pulled. * Note that caller must use pskb_may_pull() before pulling GRE header. + * + * @reason is only written when the header is rejected, so the caller has + * to initialise it before the call. + * + * A NULL @reason means that the caller is not interested in the drop + * reason, and also that the checksum must not be verified. This is what + * the ICMP error handlers need, as they only get a part of the original + * packet. */ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, - bool *csum_err, __be16 proto, int nhs) + enum skb_drop_reason *reason, __be16 proto, int nhs) {
[Severity: Low]
Does the new comment match the implementation for the NULL @reason case?
gre_parse_header() is exported, so a future caller may rely on this
contract, but the GRE_CSUM block below still calls
skb_checksum_simple_validate() unconditionally and, when it succeeds,
skb_checksum_try_convert() updates ip_summed/csum_valid on the skb:
net/ipv4/gre_demux.c:gre_parse_header() {
...
if (greh->flags & GRE_CSUM) {
if (!skb_checksum_simple_validate(skb)) {
skb_checksum_try_convert(skb, IPPROTO_GRE,
null_compute_pseudo);
} else if (reason) {
...
}
Only the error return is gated on @reason. The two NULL callers,
ipgre_err() in net/ipv4/ip_gre.c and ip6gre_err() in net/ipv6/ip6_gre.c:
if (gre_parse_header(skb, &tpi, NULL, htons(ETH_P_IP),
iph->ihl * 4) < 0)
return;
still have the checksum of their partial packet computed and the csum
state touched. This is the same behaviour as the pre-patch csum_err ==
NULL case, so nothing changes at runtime; would it be worth wording the
comment as "a checksum error is not reported" instead of "the checksum
must not be verified"?
quoted hunk ↗ jump to hunk
@@ -87,8 +104,8 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, if (!skb_checksum_simple_validate(skb)) { skb_checksum_try_convert(skb, IPPROTO_GRE, null_compute_pseudo); - } else if (csum_err) { - *csum_err = true; + } else if (reason) { + *reason = SKB_DROP_REASON_GRE_CSUM; return -EINVAL; }
[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913034937.875068-1-littlesmilingcloud%40gmail.com