Date:   Mon, 16 Jul 2018 15:27:26 -0700
From:   Nathan Harold <nharold@...gle.com>
To:     Eyal Birger <eyal.birger@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ipsec-next] xfrm: Allow Set Mark to be Updated Using UPDSA

< re-sent with apologies due to incorrect formatting last time... :-( >

Hi Eyal,

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

In the vanilla case this is true. IE, if there are no strange/abusive
uses of the API such as the test below where multiple SAs can match
the locate().

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

Yes.  Not sure if it's a bug or a feature, but it's possible for
multiple SAs to match... for a depressing example, check out
https://android-review.googlesource.com/c/kernel/tests/+/680958. There
may be cases where something like this is desired behavior that I'm
not aware of. Since this is control path, it felt to me like the
formalism of using the xfrm_state_bump_genids() was worth not possibly
walking into a different subtle bug later.

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

The set_mark (output_mark) is somewhat special because changing this
mark impacts the routing lookup, which up to now, none of the other
parameters in the update_sa function do. A new output_mark can and
will reroute packets to different interfaces. Thus, when we change
this thing, we want to ensure that we always build a new bundle with a
new bundle with a new route lookup based on the new set_mark. Since we
removed the flow cache, things might *incidentally* seem to work right
now; but, I think that's incidental rather than correct. By bumping
the genid, we get the dst_entry->check() function to correctly return
that the dst is obsolete when we call check(). I'm honestly not sure
what corner cases we could land in if we didn't bump the genid in such
a case.

There's definitely a lot going on behind the scenes in this little
change that I only tenuously grasp, so it's possible that I'm being
overly cautious in this case. Please let me know your further thoughts
on whether we need to bump the genid. FYI once this patch is settled,
I plan to upload a patch to update the xfrm_if_id, which I planned to
nestle in to this same logic (and with similar, albeit possibly
more-straightforward rationale).

-Nathan

On Mon, Jul 2, 2018 at 10:14 PM, Eyal Birger <eyal.birger@...il.com> wrote:
> Hi Nathan,
>
> On Fri, 29 Jun 2018 15:07:10 -0700
> Nathan Harold <nharold@...gle.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@...gle.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.

Powered by blists - more mailing lists