Re: [PATCH 13/14] docs: xfrm: include mark in XFRM_MSG_MIGRATE_STATE EEXIST tuple

[netdev-bot+sashiko@kernel.org]

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 new wildcard-shadowing text in the Error Handling section of
  Documentation/networking/xfrm/xfrm_migrate_state.rst does not say that…

--

docs: xfrm: include mark in XFRM_MSG_MIGRATE_STATE EEXIST tuple

This patch updates the XFRM_MSG_MIGRATE_STATE documentation. Mark is now
listed as part of the tuple that can return -EEXIST. The SA lookup text now
describes the exact mark/mask match done by xfrm_state_lookup_exact(). The
error handling section also gains a description of wildcard shadowing.

> diff --git a/Documentation/networking/xfrm/xfrm_migrate_state.rst b/Documentation/networking/xfrm/xfrm_migrate_state.rst
> index 9d53cb22b0075..ca2c9e5aaf33a 100644
> --- a/Documentation/networking/xfrm/xfrm_migrate_state.rst
> +++ b/Documentation/networking/xfrm/xfrm_migrate_state.rst

[ ... ]

> @@ -259,8 +264,12 @@ Attributes in the notification
>  Error Handling
>  ==============
>  
> -If the target SA tuple (new daddr, SPI, proto, new family) is already
> -occupied, the operation returns ``-EEXIST`` before the migration begins.
> +If the target SA tuple (new daddr, SPI, proto, new family, mark) is
> +already occupied, the operation returns ``-EEXIST`` before the migration
> +begins. "Occupied" includes wildcard shadowing: an existing SA with a
> +broader mask (e.g. mark 0/0) claims every mark value, so it blocks
> +migrating to any more specific mark at the same tuple, not just an
> +exact mark/mask duplicate.

[Severity: Low]
Should this text say that the SA being migrated is excluded from the check?

The SA being migrated is also an existing SA. Read literally, the new text
says that a lone 0/0 SA cannot be given a specific mark at the same daddr,
SPI, proto and family, and that the request returns -EEXIST.

The code allows this. In xfrm_do_migrate_state(), the source SA x is passed
in as self:

net/xfrm/xfrm_user.c:xfrm_do_migrate_state() {
    ...
		if (xfrm_state_mark_collides(net, new_mark->v & new_mark->m,
					     &m.new_daddr, um->id.spi,
					     um->id.proto, m.new_family, x)) {
    ...
}

xfrm_state_mark_collides() only looks at entries where x != self. Its
comment says:

/* True if some OTHER state at this tuple would wildcard-match "mark".
 * Used by MIGRATE_STATE, which must exclude the state being migrated.
 */

Because of this, the source SA's own 0/0 wildcard never causes a rejection.
The migration then goes ahead, and the new SA is installed with the
requested mark.

Could the wording say that the conflict is with another SA at the target
tuple, for example "another existing SA with a broader mask"?

>  The old SA remains intact and the operation is safe to retry after
>  resolving the conflict.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com