Re: [PATCH net-next v2 3/8] gre: make gre_parse_header() report a drop reason

[netdev-bot+sashiko]

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.

> 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"?

> @@ -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