mbox series

[ipsec,v2,0/2] xfrm: respect ip proto rules criteria in xfrm dst lookups

Message ID 20240902110719.502566-1-eyal.birger@gmail.com (mailing list archive)
Headers show
Series xfrm: respect ip proto rules criteria in xfrm dst lookups | expand

Message

Eyal Birger Sept. 2, 2024, 11:07 a.m. UTC
This series fixes the route lookup when done for xfrm to regard
L4 criteria specified in ip rules.

The first patch is a minor refactor to allow passing more parameters
to dst lookup functions.
The second patch actually passes L4 information to these lookup functions.

Signed-off-by: Eyal Birger <eyal.birger@gmail.com>

---

v2: fix first patch based on reviews from Steffen Klassert and
    Simon Horman

Eyal Birger (2):
  xfrm: extract dst lookup parameters into a struct
  xfrm: respect ip protocols rules criteria when performing dst lookups

 include/net/xfrm.h      | 28 ++++++++++++-----------
 net/ipv4/xfrm4_policy.c | 40 +++++++++++++++------------------
 net/ipv6/xfrm6_policy.c | 31 +++++++++++++-------------
 net/xfrm/xfrm_device.c  | 11 ++++++---
 net/xfrm/xfrm_policy.c  | 49 +++++++++++++++++++++++++++++++----------
 5 files changed, 94 insertions(+), 65 deletions(-)

Comments

Antony Antony Sept. 2, 2024, 1:52 p.m. UTC | #1
On Mon, Sep 02, 2024 at 04:07:17AM -0700, Eyal Birger via Devel wrote:
> This series fixes the route lookup when done for xfrm to regard
> L4 criteria specified in ip rules.

Hi Eyal,
This isn't a review of the patch set, instead curiosity about use cases.
This sounds interesting. Would you like to elaborate on the use cases 
supported in this patch? From what I understand so far, it seems related to 
'ip rule', but I'm wondering about possible use cases: inner packet routing 
rule of tunnel? May be you could explain it at the IPsec coffee hour or 
share some use case or test script.

Is this only for route based IPsec, i.e. with xfrmi interface, or also for a 
policy based without route use cases. In the later case there were 
discussions why do we need a route for the inner packet.

-antony

> 
> The first patch is a minor refactor to allow passing more parameters
> to dst lookup functions.
> The second patch actually passes L4 information to these lookup functions.
> 
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> 
> ---
> 
> v2: fix first patch based on reviews from Steffen Klassert and
>     Simon Horman
> 
> Eyal Birger (2):
>   xfrm: extract dst lookup parameters into a struct
>   xfrm: respect ip protocols rules criteria when performing dst lookups
> 
>  include/net/xfrm.h      | 28 ++++++++++++-----------
>  net/ipv4/xfrm4_policy.c | 40 +++++++++++++++------------------
>  net/ipv6/xfrm6_policy.c | 31 +++++++++++++-------------
>  net/xfrm/xfrm_device.c  | 11 ++++++---
>  net/xfrm/xfrm_policy.c  | 49 +++++++++++++++++++++++++++++++----------
>  5 files changed, 94 insertions(+), 65 deletions(-)
> 
> -- 
> 2.34.1
> 
> -- 
> Devel mailing list
> Devel@linux-ipsec.org
> https://linux-ipsec.org/mailman/listinfo/devel
Antony Antony Sept. 2, 2024, 8:39 p.m. UTC | #2
On Mon, Sep 02, 2024 at 04:07:17AM -0700, Eyal Birger via Devel wrote:
> This series fixes the route lookup when done for xfrm to regard
> L4 criteria specified in ip rules.

Thanks Eyal for explaining the purpose of this series on the call.
How about something like this for the beginning of the commit message:

'This series fixes the route lookup for the outer packet after
encapsulation, including the L4 criteria specified in IP rules.'

It's just a cosmetic suggestion, so may be improve it if you're planning to
send a new version of the patch series for other reasons.

We ran into this issue before and used workaround, mark instead of L4 in the 
"ip rule" for the outer packet.

> The first patch is a minor refactor to allow passing more parameters
> to dst lookup functions.
> The second patch actually passes L4 information to these lookup functions.
> 
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>

Tested-by: Antony Antony <antony.antony@secunet.com>

And I have a further suggestion to improve this fix make it more generic.  

I was doing the following rule as a work around for ESP-in-UDP tunnels.
ip rule add from all to 192.1.2.23 fwmark 0x1 lookup 50

With your fix I can change it to a L4 rule when using ESP-in-UDP
ip rule add from 192.1.2.45 to 192.1.2.23 ipproto udp dport 4500 lookup 50

However, when not using ESP, without UDP, and rule with "ipproto esp" does 
work.

ip rule add from 192.1.2.45 to 192.1.2.23 ipproto esp lookup 50

So, I have come up with a fix/hack on top of your fix.


@@ -327,6 +327,8 @@ static inline struct dst_entry *xfrm_dst_lookup(struct xfrm_state *x,

+       } else {
+               params.ipproto = IPPROTO_ESP;

With this fix "ipproto esp" rules also works.
see the attached full patch.

regards,
-antony