Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH v5] api: ipsec: require more packet metadata in IPsec operations #1344

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JannePeltonen
Copy link
Collaborator

Require that input packets to IPsec operations have valid L3 type
and L4 offset (except in outbound tunnel mode without L4 checksum
offload) metadata in addition to L3 offset.

This can help implementations to be more efficient as they no
longer need to parse the IP header.

This also resolves the ambiguity on whether possible destination
options extension headers are to be included or excluded from IPsec
encapsulation (the RFCs allow both) in transport mode. Now the
provided L4 offset explicitly indicates the part of the packet to
follow the IPsec header.

Signed-off-by: Janne Peltonen janne.peltonen@nokia.com

@odpbuild odpbuild changed the title api: ipsec: require more packet metadata in IPsec operations [PATCH v1] api: ipsec: require more packet metadata in IPsec operations Jul 19, 2021
@nithind1988
Copy link
Contributor

Looks good to me.
Reviewed-by: Nithin Dabilpuram ndabilpuram@marvell.com

@vvelumuri @asasidharan @anoobj

@odpbuild odpbuild changed the title [PATCH v1] api: ipsec: require more packet metadata in IPsec operations [PATCH v2] api: ipsec: require more packet metadata in IPsec operations Aug 24, 2021
@JannePeltonen
Copy link
Collaborator Author

v2: Clarified when L4 offset does not need to be set, as proposed by @psavol .

@@ -1688,7 +1689,15 @@ int odp_ipsec_in(const odp_packet_t pkt_in[], int num_in,
* Each input packet must have a valid value for these metadata (other metadata
* is ignored):
* - L3 offset: Offset to the first byte of the (outmost) IP header
* - L4 offset: Offset to the L4 header if L4 checksum offload is requested
* - L3 type: Type of the IP packet (ODP_PROTO_L3_TYPE_IPv4 or _IPv6).
* Ignored if the tfc_dummy flag is set.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no direct way of setting L3 type like there is for L3 offset (i.e. odp_packet_l3_offset_set()). So an application cannot set the L3 type for locally originated packets in any way other than by calling odp_packet_parse() which is heavy and seems to defeat the purpose of this change.

I am not sure if making L3 type settable and thus changing it from mere parse result to more general metadata would be good. One option would be to just drop the requirement of setting the L3 type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odp_packet_has_ipv6_set() or odp_packet_has_ipv4_set() is already available right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as discussed, that could be used but it probably requires some clarification in the API spec.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would good to mention explicitly how L3 type is set (use odp_packet_has_ipv4_set()...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just drop the requirement for L3 type since the way to setting L3 type is non-obvious and it may actually require more than one call (e.g. to set IPv4 flag and to clear IPv6 flag). At least SW based implementations can just about as easily figure out L3 type from the IP header which is likely in the cache.

@nithind1988, is it ok for you if we remote the L3 type requirement from both functions? If yes, I will take it away from the next version and add your reviewed-by tag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep it given that for packets that are being forwarded, there is no need to set the L3 type explicitly again ?
I guess the concern is only when packet is being created newly by application.

And with respect meta data change, I guess the text can be like that is present for PKTIO Tx checksum generation.
and isn't it implicit that has IPv4 set => has_ipv6 is automatically cleared as both cannot coexist in single layer

`

  • For correct operation, packet metadata must provide valid offsets and type
  • flags for the appropriate layer 3 and layer 4 protocols. L3 and L4 offsets
  • can be updated with odp_packet_l3_offset_set() and odp_packet_l4_offset_set()
  • calls. L3 and L4 type flags can be updated using odp_packet_has_*_set() calls
  • For example, UDP checksum calculation needs both L3 and L4 types (IP and UDP) and
  • L3 and L4 offsets (to access IP and UDP headers), while IP checksum
  • calculation only needs L3 type (IP) and L3 offset (to access IP header).
  • When application (e.g. a switch) does not modify L3/L4 data and thus checksum
  • does not need to be updated, checksum insertion should be disabled for optimal
  • performance.

`

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for packets that are being forwarded, there is no need to set the L3 type explicitly again ?

The fact that a packet is forwarded (and inbound packet parsing is enabled) is not sufficient. There must not be tunneling (e.g. IPv6 carried in any manner inside IPv4 or vice versa) or protocol conversion (e.g. from non-IP to IP) involved. And in some applications the majority of packets may be effectively locally originated.

I guess the text can be like that is present for PKTIO Tx checksum generation.

I would either remove the L3 type requirement, or add this sentence: "L3 type can be updated using odp_packet_has_*_set() calls."

isn't it implicit that has IPv4 set => has_ipv6 is automatically cleared as both cannot coexist in single layer

I do not think it is since linux-gen ODP and ODP-DPDK do not work that way. Maybe (?) the API could be changed so that setting one flag in a layer resets the other flags of the same layer.

But is there really some tangible benefit in requiring the L3 type? If not then maybe we do not need to add it now. I understood that you just need L4 offset for speeding up your implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is there really some tangible benefit in requiring the L3 type? If not then maybe we do not need to add it now. I understood that you just need L4 offset for speeding up your implementation.

Ok. I was taking w.r.t general semantics which could be useful for other implementations

I would either remove the L3 type requirement, or add this sentence: "L3 type can be updated using odp_packet_has_*_set() calls."

Yes, l4 offset is sufficient for us for outbound inline for today. So we are fine either way.

@JannePeltonen
Copy link
Collaborator Author

It seems to me that L4 type should be required too for outbound transport mode, at least for IPv6. Having just L4 offset readily available does not help the implementation avoid parsing the whole set of IPv6 headers since the payload protocol needs to be known for transport mode (so it can be written in the next header field of the ESP trailer). Or would this API change be useful to you with just L4 offset without L4 type?

If L4 type is required, there is the problem that there is no generic L4 type metadata but a bunch of L4 flags for the most common types. It would not be possible to apply IPsec to other payload protocols. An alternative would be pass the L4 type (i.e. the IPsec payload protocol) through IPsec output operation parameters. It would probably have to be in odp_ipsec_out_opt_t. It looks a bit messy. Yet another alternative would be to scratch this API change and keep the current API that requires only L3 offset and makes the implementation parse the headers.

@nithind1988
Copy link
Contributor

It seems to me that L4 type should be required too for outbound transport mode, at least for IPv6. Having just L4 offset readily available does not help the implementation avoid parsing the whole set of IPv6 headers since the payload protocol needs to be known for transport mode (so it can be written in the next header field of the ESP trailer). Or would this API change be useful to you with just L4 offset without L4 type?

If L4 type is required, there is the problem that there is no generic L4 type metadata but a bunch of L4 flags for the most common types. It would not be possible to apply IPsec to other payload protocols. An alternative would be pass the L4 type (i.e. the IPsec payload protocol) through IPsec output operation parameters. It would probably have to be in odp_ipsec_out_opt_t. It looks a bit messy. Yet another alternative would be to scratch this API change and keep the current API that requires only L3 offset and makes the implementation parse the headers.

For our platform l4 type is not needed and l4 offset is sufficient because our HW does read packet content from L3 header. By this spec change, we can avoid reading/parsing the packet in SW before submitting to HW. And the requirement for knowing l4 offset comes from the fact that we need to know in SW what is the final packet size post encryption to be able to get it ready for transmission on NIX.
I agree that it probably doesn't make any difference in linux-generic if l4 type is not given but even part of metadata might be useful for HW platforms. In future if some platform l4 type as well then we can think of expanding the spec ?

Also regarding

Now the provided L4 offset explicitly indicates the part of the packet to
follow the IPsec header.

Can we have a capability field as well to indicate if a platform supports it ? Today, our platform always inserts ipsec header in transport mode at a predefined offset based on RFC 4303 and we cannot really provide offset per pkt.

Also, ideally if we are talking about L4 offset, then pointing it to a IPv6 extension header doesn't align with the spec ? Maybe it should be some other offset in this case and L4 should always point to protocol after IPv6 such as TCP/UDP etc.

@JannePeltonen
Copy link
Collaborator Author

I still wonder where you would like the L4 offset to point to in IPv6 transport mode when the packet has a destination options extension header. To the destination option extension header or to the actual L4 protocol header?

The RFC says that destination options extension header can appear in a transport mode packet before or after the IPsec header. Which way is it in your implementation currently? The IPsec header cannot be at a predefined offset from the start of the IP packets because of the varying number of bytes in possible extension headers that are to be placed before the IPsec header.

You mentioned that L4 offset would help your implementation to figure out the final length of the ESP packet. If the destination options extension header is put after the ESP header in your implementation, then I would think that you would need L4 offset to point to the extension header and not the final L4 payload or otherwise your length calculation based on the L4 offset would be off.

@nithind1988
Copy link
Contributor

In our HW, we have the following modes choosable able per SA
Mode #1

  • Mobility header is not supported.
  • If packet has a destination header as last extension header, then ESP will be inserted before that destination options header. Otherwise, ESP header will be inserted just before L4 header.

Mode #2

  • Mobility header is supported
  • Without mobility header, ESP will be always inserted before L4 header.
  • If packet has mobility header, then ESP will be inserted before mobility header. Destination options header will never be part of ESP payload with or without mobility header present.

Based on RFC https://datatracker.ietf.org/doc/html/rfc3776, Mobility header should be part of ESP payload and Destination options outside ESP payload which is mode #2
Our platform can default to mode #2 given today there is no requirement or SA params to choose such a mode.
And L4 offset can point to either L4 header or mobility header in our case.

@JannePeltonen
Copy link
Collaborator Author

JannePeltonen commented Sep 21, 2021

Based on RFC https://datatracker.ietf.org/doc/html/rfc3776, Mobility header should be part of ESP payload and Destination options outside ESP payload which is mode #2

The destination options header that carries the home address option should indeed come before ESP, but that does not mean that other destination options in other contexts than MIPv6 would not be protected by ESP.

Anyway, I think we could leave the L4 offset requirement pretty much as currently written in this PR, even if it can then express things that are not supported by all implementations. And I think from input side we can remove all additional requirements beside the L3 offset, as discussed.

@nithind1988
Copy link
Contributor

Anyway, I think we could leave the L4 offset requirement pretty much as currently written in this PR, even if it can then express things that are not supported by all implementations. And I think from input side we can remove all additional requirements beside the L3 offset, as discussed.

Ack.

@odpbuild odpbuild changed the title [PATCH v2] api: ipsec: require more packet metadata in IPsec operations [PATCH v3] api: ipsec: require more packet metadata in IPsec operations Sep 28, 2021
@JannePeltonen
Copy link
Collaborator Author

v3:

  • Small API tweaks (L4 offset not required for non-UDP-encapsulated inbound packets)
  • Included validation and perf test program changes to comply with the changed API

@odpbuild odpbuild changed the title [PATCH v3] api: ipsec: require more packet metadata in IPsec operations [PATCH v4] api: ipsec: require more packet metadata in IPsec operations Oct 1, 2021
@JannePeltonen
Copy link
Collaborator Author

v4: rebased

@nithind1988
Copy link
Contributor

Reviewed-by: Nithin Dabilpuram ndabilpuram@marvell.com

Require that input packets to outbound IPsec operations have valid
L4 offset (except in tunnel mode without L4 checksum offload) metadata
in addition to L3 offset.

This can help some implementations to be more efficient by reducing
the need to parse the IP header in SW.

Clarify that L4 offset is required for inbound packets only when
they are UDP encapsulated.

Signed-off-by: Janne Peltonen <janne.peltonen@nokia.com>
Set l4_offset metadata in locally created packets. The metadata was
already set in received packet through the default parse config in pktio.

Signed-off-by: Janne Peltonen <janne.peltonen@nokia.com>
@odpbuild odpbuild changed the title [PATCH v4] api: ipsec: require more packet metadata in IPsec operations [PATCH v5] api: ipsec: require more packet metadata in IPsec operations Nov 9, 2021
@JannePeltonen
Copy link
Collaborator Author

v5: dropped L3 type requirement and the corresponding changes.

@nithind1988
Copy link
Contributor

Reviewed-by: Nithin Dabilpuram ndabilpuram@marvell.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants