Re: [PATCH net-next v4 01/10] ipv6: add ip6_del_rt_reason()

>And be aware of:
>https://lore.kernel.org/all/83360de7addb13a3b5f4d5e722148f248fdb2ae0.1784884817.git.pabeni@redhat.com/

Thanks for the notice, I will proactively check the AI comment on going forward.

> From Sashiko:
>
> "
> This isn't a bug, but was carrying the reason in struct nl_info considered
> instead of adding a parallel argument?
> "
>
> I did consider it, but it doesn't look like a good fit to carry a route
> deletion reason in a generic netlink structure (also used by nexthops).

Acked. I also considered this path but also think put the field inside
struct nl_info seems to be a wrong design.

> This isn't a bug, but since these names become uAPI on first release, is
> the RTA_ prefix the right choice for the payload values?
> RTA_ is the established prefix for route attribute ids in enum
> rtattr_type_t, and RTA_DEL_REASON is added to that very enum a few lines
> above. So RTA_DEL_REASON_EXPIRED reads like an attribute id, and
> RTA_DEL_REASON_MAX visually parallels RTA_MAX while meaning something
> completely different. Elsewhere the payload value space of an attribute
> gets its own prefix, for instance LWTUNNEL_ENCAP_* for RTA_ENCAP_TYPE.
> [....]
> RTA_DEL_REASON_RA_WITHDRAWN is an IPv6-specific cause placed in the
> family-agnostic route attribute value space [...] Would it be worth
> stating in the comment that the value space must never be reinterpreted
> per family [...]
> "

Will fix it in patch v5.

> It's weird to have 'skip_notify' in a function that is supposed to
> notify the deletion reason to user space. All the callers except
> ip6_del_rt() pass 'false'. Can ip6_del_rt() call __ip6_del_rt() with
> 'RTA_DEL_REASON_UNSPEC' ?

Will fix it in patch v5.

>This is a pre-existing issue, but does rt6_nh_dump_exceptions() safely iterate
>over the exception list?
>Looking at the loop containing this code:
>net/ipv6/route.c:rt6_nh_dump_exceptions() {
>...

I checked other Sashiko comments, and maybe the above one is worth a
follow-up? I'll see if I can get a KSAN trace on it first.

Thanks,

Yuyang