From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eyal Birger <eyal.birger@gmail.com>
Subject: Re: [PATCH ipsec-next] xfrm: Allow Set Mark to be Updated Using
 UPDSA
Date: Tue, 3 Jul 2018 08:14:16 +0300
Message-ID: <20180703081416.04c9395f@jimi>
References: <20180629220710.190783-1-nharold@google.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org
To: Nathan Harold <nharold@google.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-wr0-f195.google.com ([209.85.128.195]:45462 "EHLO
        mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753484AbeGCFOX (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 3 Jul 2018 01:14:23 -0400
Received: by mail-wr0-f195.google.com with SMTP id u7-v6so503267wrn.12
        for <netdev@vger.kernel.org>; Mon, 02 Jul 2018 22:14:22 -0700 (PDT)
In-Reply-To: <20180629220710.190783-1-nharold@google.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

Hi Nathan,

On Fri, 29 Jun 2018 15:07:10 -0700
Nathan Harold <nharold@google.com> wrote:

> Allow UPDSA to change "set mark" to permit
> policy separation of packet routing decisions from
> SA keying in systems that use mark-based routing.
> 
> The set mark, used as a routing and firewall mark
> for outbound packets, is made update-able which
> allows routing decisions to be handled independently
> of keying/SA creation. To maintain consistency with
> other optional attributes, the set mark is only
> updated if sent with a non-zero value.
> 
> The per-SA lock and the xfrm_state_lock are taken in
> that order to avoid a deadlock with
> xfrm_timer_handler(), which also takes the locks in
> that order.
> 
> Signed-off-by: Nathan Harold <nharold@google.com>
> Change-Id: Ia05c6733a94c1901cd1e54eb7c7e237704678d71
> ---
>  net/xfrm/xfrm_state.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index e04a510ec992..c9ffcdfa89f6 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1562,6 +1562,15 @@ int xfrm_state_update(struct xfrm_state *x)
>  		if (x1->curlft.use_time)
>  			xfrm_state_check_expire(x1);
>  
> +		if (x->props.smark.m || x->props.smark.v) {
> +			spin_lock_bh(&net->xfrm.xfrm_state_lock);
> +
> +			x1->props.smark = x->props.smark;
> +
> +			__xfrm_state_bump_genids(x1);

So I'm trying to wrap my head around this genid thing :)

If x1 points to a state previously found using __xfrm_state_locate(x),
won't __xfrm_state_bump_genids(x1) be equivalent to x1->genid++ in
this case?

Is it possible that other states will match all of x1 parameters?

Also, any idea why this isn't needed for other changes in the state?

Thanks!
Eyal.