Re: [PATCH v5 25/29] ndp: use iov_tail rather than pool

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 17 Apr 2025 18:51:32 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  ndp.c | 16 ++++++++--------
>  ndp.h |  4 ++--
>  tap.c | 10 +++-------
>  3 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/ndp.c b/ndp.c
> index b09a1a133f56..95cb1d983f12 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -336,13 +336,17 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
>   * @c:		Execution context
>   * @ih:		ICMPv6 header
>   * @saddr:	Source IPv6 address
> - * @p:		Packet pool
> + * @data:	Single packet with ICMPv6 header
>   *
>   * Return: 0 if not handled here, 1 if handled, -1 on failure
>   */
> -int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> -	const struct in6_addr *saddr, const struct pool *p)
> +int ndp(const struct ctx *c, const struct in6_addr *saddr,
> +	struct iov_tail *data)
>  {
> +	struct icmp6hdr ihc;
> +	const struct icmp6hdr *ih;

Nit: these two should be swapped. Usual comment about the 'c' (here
it's particularly unclear, I think, but also the 'nsc' usage below is
rather obscure).

> +
> +	ih = IOV_PEEK_HEADER(data, ihc);
>  	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
>  		return 0;

I had started splitting warnings reported by Coverity per patch but
actually it's all about cases like this one, so here it is as a single
comment:

/home/sbrivio/passt/ndp.c:351:2:
  Type: Dereference null return value (NULL_RETURNS)

/home/sbrivio/passt/ndp.c:350:2: Call to null-returning function
  1. returned_null: "iov_peek_header_" returns "NULL" (checked 18 out of 20 times).
/home/sbrivio/passt/iov.c:315:2: Call to null-returning function
  1.1. path: Condition "p", taking false branch.
/home/sbrivio/passt/iov.c:319:2:
  1.2. path: Condition "l != len", taking true branch.
/home/sbrivio/passt/iov.c:320:3:
  1.3. return_null: Explicitly returning null.
/home/sbrivio/passt/ndp.c:350:2:
  2. var_assigned: Assigning: "ih" = "NULL" return value from "iov_peek_header_".
/home/sbrivio/passt/ndp.c:351:2:
  3. dereference: Dereferencing "ih", which is known to be "NULL".
/home/sbrivio/passt/dhcp.c:320:2: Examples where return value was checked for null
  4. example_assign: Example 1: Assigning: "iph" = return value from "iov_peek_header_(data, &iphc, 20UL, 4UL)".
/home/sbrivio/passt/dhcp.c:321:2:
  5. example_checked: Example 1 (cont.): "iph" has its value checked in "iph".
/home/sbrivio/passt/dhcpv6.c:296:2: Examples where return value was checked for null
  6. example_checked: Example 2: "iov_peek_header_(data, &oc, 4UL, 1UL)" has its value checked in "o = (struct opt_hdr *)iov_peek_header_(data, &oc, 4UL, 1UL)".
/home/sbrivio/passt/iov.c:343:2: Examples where return value was checked for null
  7. example_assign: Example 3: Assigning: "p" = return value from "iov_peek_header_(tail, v, len, align)".
/home/sbrivio/passt/iov.c:345:2:
  8. example_checked: Example 3 (cont.): "p" has its value checked in "p".
/home/sbrivio/passt/ip.c:50:2: Examples where return value was checked for null
  9. example_checked: Example 4: "iov_peek_header_(data, &oc, 2UL, 1UL)" has its value checked in "o = (struct ipv6_opt_hdr *)iov_peek_header_(data, &oc, 2UL, 1UL)".
/home/sbrivio/passt/tap.c:776:3: Examples where return value was checked for null
  10. example_assign: Example 5: Assigning: "uh" = return value from "iov_peek_header_(&data, &uhc, 8UL, 2UL)".
/home/sbrivio/passt/tap.c:777:3:
  11. example_checked: Example 5 (cont.): "uh" has its value checked in "uh".

...and I think I would actually be reasonable to add a check on 'ih'.

Same for (with this whole series applied):

/home/sbrivio/passt/dhcpv6.c:349:5:
  Type: Dereference null return value (NULL_RETURNS)

/home/sbrivio/passt/tap.c:1006:4:
  Type: Dereference null return value (NULL_RETURNS)

/home/sbrivio/passt/tap.c:1015:4:
  Type: Dereference null return value (NULL_RETURNS)

-- 
Stefano