| Message ID | bb191b37cd631341552ee87eb349f0525b90f14f.1712685187.git.antony.antony@secunet.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec-next,v9] xfrm: Add Direction to the SA in or out | expand |
Le 09/04/2024 à 19:56, Antony Antony a écrit : > This patch introduces the 'dir' attribute, 'in' or 'out', to the > xfrm_state, SA, enhancing usability by delineating the scope of values > based on direction. An input SA will now exclusively encompass values > pertinent to input, effectively segregating them from output-related > values. This change aims to streamline the configuration process and > improve the overall clarity of SA attributes. > > This feature sets the groundwork for future patches, including > the upcoming IP-TFS patch. > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > --- > v8->v9: > - add validation XFRM_STATE_ICMP not allowed on OUT SA. > > v7->v8: > - add extra validation check on replay window and seq > - XFRM_MSG_UPDSA old and new SA should match "dir" > > v6->v7: > - add replay-window check non-esn 0 and ESN 1. > - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD Why? I still think that having an 'input' SA used in the output path is wrong and confusing. Please, don't drop this check. Regards, Nicolas
2024-04-10, 08:32:20 +0200, Nicolas Dichtel wrote: > Le 09/04/2024 à 19:56, Antony Antony a écrit : > > This patch introduces the 'dir' attribute, 'in' or 'out', to the > > xfrm_state, SA, enhancing usability by delineating the scope of values > > based on direction. An input SA will now exclusively encompass values > > pertinent to input, effectively segregating them from output-related > > values. This change aims to streamline the configuration process and > > improve the overall clarity of SA attributes. > > > > This feature sets the groundwork for future patches, including > > the upcoming IP-TFS patch. > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > > --- > > v8->v9: > > - add validation XFRM_STATE_ICMP not allowed on OUT SA. > > > > v7->v8: > > - add extra validation check on replay window and seq > > - XFRM_MSG_UPDSA old and new SA should match "dir" > > > > v6->v7: > > - add replay-window check non-esn 0 and ESN 1. > > - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD > Why? I still think that having an 'input' SA used in the output path is wrong > and confusing. > Please, don't drop this check. Limiting XFRMA_SA_DIR to only HW offload makes no sense. It's completely redundant with an existing property. We should also try to limit the divergence between offload and non-offload configuration. If something is clearly only for offloaded configs, then fine, but otherwise the APIs should be identical. And based on what Antony says, this is intended in large part for IPTFS, which is not going to be offloaded any time soon (or probably ever), so that restriction would have to be lifted immediately. I'm not sure why Antony accepted your request.
Le 10/04/2024 à 09:26, Sabrina Dubroca a écrit : > 2024-04-10, 08:32:20 +0200, Nicolas Dichtel wrote: >> Le 09/04/2024 à 19:56, Antony Antony a écrit : >>> This patch introduces the 'dir' attribute, 'in' or 'out', to the >>> xfrm_state, SA, enhancing usability by delineating the scope of values >>> based on direction. An input SA will now exclusively encompass values >>> pertinent to input, effectively segregating them from output-related >>> values. This change aims to streamline the configuration process and >>> improve the overall clarity of SA attributes. >>> >>> This feature sets the groundwork for future patches, including >>> the upcoming IP-TFS patch. >>> >>> Signed-off-by: Antony Antony <antony.antony@secunet.com> >>> --- >>> v8->v9: >>> - add validation XFRM_STATE_ICMP not allowed on OUT SA. >>> >>> v7->v8: >>> - add extra validation check on replay window and seq >>> - XFRM_MSG_UPDSA old and new SA should match "dir" >>> >>> v6->v7: >>> - add replay-window check non-esn 0 and ESN 1. >>> - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD >> Why? I still think that having an 'input' SA used in the output path is wrong >> and confusing. >> Please, don't drop this check. > > Limiting XFRMA_SA_DIR to only HW offload makes no sense. It's > completely redundant with an existing property. We should also try to > limit the divergence between offload and non-offload configuration. If Sure. > something is clearly only for offloaded configs, then fine, but > otherwise the APIs should be identical. But right now, the property is enforced for offload and but not for non-offload. In that sense, the api is not identical. I'm only asking to make this explicit. > > And based on what Antony says, this is intended in large part for > IPTFS, which is not going to be offloaded any time soon (or probably > ever), so that restriction would have to be lifted immediately. I'm > not sure why Antony accepted your request. I don't see the problem with that. The attribute can be relaxed later for IPTFS if needed. But there are use cases without offload and without IPTFS. Why isn't it possible to restrict the use of an input SA to the input path and output SA to xmit path? Regards, Nicolas
2024-04-10, 09:35:08 +0200, Nicolas Dichtel wrote: > Le 10/04/2024 à 09:26, Sabrina Dubroca a écrit : > > 2024-04-10, 08:32:20 +0200, Nicolas Dichtel wrote: > >> Le 09/04/2024 à 19:56, Antony Antony a écrit : > >>> v6->v7: > >>> - add replay-window check non-esn 0 and ESN 1. > >>> - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD > >> Why? I still think that having an 'input' SA used in the output path is wrong > >> and confusing. > >> Please, don't drop this check. > > > > Limiting XFRMA_SA_DIR to only HW offload makes no sense. It's > > completely redundant with an existing property. We should also try to > > limit the divergence between offload and non-offload configuration. If > Sure. > > > something is clearly only for offloaded configs, then fine, but > > otherwise the APIs should be identical. > But right now, the property is enforced for offload and but not for non-offload. > In that sense, the api is not identical. I'm only asking to make this explicit. We can't get rid of the offload-specific way of setting the direction, because it's a flag (off = out, on = in), but if we add another way of setting the direction, it should be for all cases (we already have one for offload, we don't need a 2nd offload-specific flag), and it should correctly lock down uses (incompatible options, and use of the SA in the datapath as you said). > > And based on what Antony says, this is intended in large part for > > IPTFS, which is not going to be offloaded any time soon (or probably > > ever), so that restriction would have to be lifted immediately. I'm > > not sure why Antony accepted your request. > I don't see the problem with that. The attribute can be relaxed later for IPTFS > if needed. Then we would have landed back on v4 (unless we add the checks we're discussing now)... > But there are use cases without offload and without IPTFS. Sure. That's probably the vast majority of IPsec users. > Why isn't it possible to restrict the use of an input SA to the input path and > output SA to xmit path? Because nobody has written a patch for it yet :)
Le 10/04/2024 à 10:17, Sabrina Dubroca a écrit : [snip] >> Why isn't it possible to restrict the use of an input SA to the input path and >> output SA to xmit path? > > Because nobody has written a patch for it yet :) > For me, it should be done in this patch/series ;-)
2024-04-10, 10:37:27 +0200, Nicolas Dichtel wrote: > Le 10/04/2024 à 10:17, Sabrina Dubroca a écrit : > [snip] > >> Why isn't it possible to restrict the use of an input SA to the input path and > >> output SA to xmit path? > > > > Because nobody has written a patch for it yet :) > > > For me, it should be done in this patch/series ;-) Sounds good to me.
On Wed, 10 Apr 2024, Sabrina Dubroca via Devel wrote: > 2024-04-10, 10:37:27 +0200, Nicolas Dichtel wrote: >> Le 10/04/2024 à 10:17, Sabrina Dubroca a écrit : >> [snip] >> >> Why isn't it possible to restrict the use of an input SA to the input path and >> >> output SA to xmit path? >> > >> > Because nobody has written a patch for it yet :) >> > >> For me, it should be done in this patch/series ;-) > > Sounds good to me. If this is not the case currently, what happens when our own generated SPI clashes with a peer generated SPI? Could it be we end up using the wrong SA state? Paul
On Wed, Apr 10, 2024 at 08:32:20AM +0200, Nicolas Dichtel wrote: > Le 09/04/2024 à 19:56, Antony Antony a écrit : > > This patch introduces the 'dir' attribute, 'in' or 'out', to the > > xfrm_state, SA, enhancing usability by delineating the scope of values > > based on direction. An input SA will now exclusively encompass values > > pertinent to input, effectively segregating them from output-related > > values. This change aims to streamline the configuration process and > > improve the overall clarity of SA attributes. > > > > This feature sets the groundwork for future patches, including > > the upcoming IP-TFS patch. > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > > --- > > v8->v9: > > - add validation XFRM_STATE_ICMP not allowed on OUT SA. > > > > v7->v8: > > - add extra validation check on replay window and seq > > - XFRM_MSG_UPDSA old and new SA should match "dir" > > > > v6->v7: > > - add replay-window check non-esn 0 and ESN 1. > > - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD > Why? I still think that having an 'input' SA used in the output path is wrong > and confusing. I don't think this can happen. This patch does not change the state lookups, so we should match the correct state as it was before that patch. On the long run, we should make the direction a lookup key. That should have happened with the initial implemenatation already, now ~25 years later we would have to maintain the old input/output combined SADB and two new ones where input and output states are separated.
On Wed, Apr 10, 2024 at 10:37:27AM +0200, Nicolas Dichtel wrote: > Le 10/04/2024 à 10:17, Sabrina Dubroca a écrit : > [snip] > >> Why isn't it possible to restrict the use of an input SA to the input path and > >> output SA to xmit path? > > > > Because nobody has written a patch for it yet :) > > > For me, it should be done in this patch/series ;-) I tend to disagree here. Adding the direction as a lookup key is IMO beyond the scope of this patch. That's complicated and would defer this series by months. Given that the upcomming IPTFS implementation has a lot of direction specific config options, it makes sense to take that this patch now. Otherwise we have the direction specific options in input and output states forever.
On Wed, Apr 10, 2024 at 09:52:36AM -0400, Paul Wouters wrote: > On Wed, 10 Apr 2024, Sabrina Dubroca via Devel wrote: > > > 2024-04-10, 10:37:27 +0200, Nicolas Dichtel wrote: > > > Le 10/04/2024 à 10:17, Sabrina Dubroca a écrit : > > > [snip] > > > >> Why isn't it possible to restrict the use of an input SA to the input path and > > > >> output SA to xmit path? > > > > > Because nobody has written a patch for it yet :) > > > > For me, it should be done in this patch/series ;-) > > > > Sounds good to me. > > If this is not the case currently, what happens when our own generated > SPI clashes with a peer generated SPI? Could it be we end up using the > wrong SA state? No, the kernel will reject to insert a second identical SPI.
Le 11/04/2024 à 09:14, Steffen Klassert a écrit : > On Wed, Apr 10, 2024 at 08:32:20AM +0200, Nicolas Dichtel wrote: >> Le 09/04/2024 à 19:56, Antony Antony a écrit : >>> This patch introduces the 'dir' attribute, 'in' or 'out', to the >>> xfrm_state, SA, enhancing usability by delineating the scope of values >>> based on direction. An input SA will now exclusively encompass values >>> pertinent to input, effectively segregating them from output-related >>> values. This change aims to streamline the configuration process and >>> improve the overall clarity of SA attributes. >>> >>> This feature sets the groundwork for future patches, including >>> the upcoming IP-TFS patch. >>> >>> Signed-off-by: Antony Antony <antony.antony@secunet.com> >>> --- >>> v8->v9: >>> - add validation XFRM_STATE_ICMP not allowed on OUT SA. >>> >>> v7->v8: >>> - add extra validation check on replay window and seq >>> - XFRM_MSG_UPDSA old and new SA should match "dir" >>> >>> v6->v7: >>> - add replay-window check non-esn 0 and ESN 1. >>> - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD >> Why? I still think that having an 'input' SA used in the output path is wrong >> and confusing. > > I don't think this can happen. This patch does not change the > state lookups, so we should match the correct state as it was > before that patch. This is the point. The user can set whatever direction in the SA, there is no check. He can set the dir to 'output' even if the SA is used in input. > > On the long run, we should make the direction a lookup key. > That should have happened with the initial implemenatation > already, now ~25 years later we would have to maintain the > old input/output combined SADB and two new ones where input > and output states are separated. >
On Thu, Apr 11, 2024 at 11:01:32AM +0200, Nicolas Dichtel wrote: > > > Le 11/04/2024 à 09:14, Steffen Klassert a écrit : > > On Wed, Apr 10, 2024 at 08:32:20AM +0200, Nicolas Dichtel wrote: > >> Le 09/04/2024 à 19:56, Antony Antony a écrit : > >>> This patch introduces the 'dir' attribute, 'in' or 'out', to the > >>> xfrm_state, SA, enhancing usability by delineating the scope of values > >>> based on direction. An input SA will now exclusively encompass values > >>> pertinent to input, effectively segregating them from output-related > >>> values. This change aims to streamline the configuration process and > >>> improve the overall clarity of SA attributes. > >>> > >>> This feature sets the groundwork for future patches, including > >>> the upcoming IP-TFS patch. > >>> > >>> Signed-off-by: Antony Antony <antony.antony@secunet.com> > >>> --- > >>> v8->v9: > >>> - add validation XFRM_STATE_ICMP not allowed on OUT SA. > >>> > >>> v7->v8: > >>> - add extra validation check on replay window and seq > >>> - XFRM_MSG_UPDSA old and new SA should match "dir" > >>> > >>> v6->v7: > >>> - add replay-window check non-esn 0 and ESN 1. > >>> - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD > >> Why? I still think that having an 'input' SA used in the output path is wrong > >> and confusing. > > > > I don't think this can happen. This patch does not change the > > state lookups, so we should match the correct state as it was > > before that patch. > This is the point. The user can set whatever direction in the SA, there is no > check. He can set the dir to 'output' even if the SA is used in input. Ah, yes indeed. That should be fixed, thanks for clarification!
Le 11/04/2024 à 09:22, Steffen Klassert a écrit : > On Wed, Apr 10, 2024 at 10:37:27AM +0200, Nicolas Dichtel wrote: >> Le 10/04/2024 à 10:17, Sabrina Dubroca a écrit : >> [snip] >>>> Why isn't it possible to restrict the use of an input SA to the input path and >>>> output SA to xmit path? >>> >>> Because nobody has written a patch for it yet :) >>> >> For me, it should be done in this patch/series ;-) > > I tend to disagree here. Adding the direction as a lookup key > is IMO beyond the scope of this patch. That's complicated and > would defer this series by months. Given that the upcomming IPTFS > implementation has a lot of direction specific config options, > it makes sense to take that this patch now. Otherwise we have the > direction specific options in input and output states forever. I don't understand why the direction could not be mandatory and checked for new options only (offload, iptfs, etc.) and reject for legacy use cases.
On Thu, Apr 11, 2024 at 11:05:02AM +0200, Nicolas Dichtel wrote: > Le 11/04/2024 à 09:22, Steffen Klassert a écrit : > > On Wed, Apr 10, 2024 at 10:37:27AM +0200, Nicolas Dichtel wrote: > >> Le 10/04/2024 à 10:17, Sabrina Dubroca a écrit : > >> [snip] > >>>> Why isn't it possible to restrict the use of an input SA to the input path and > >>>> output SA to xmit path? > >>> > >>> Because nobody has written a patch for it yet :) > >>> > >> For me, it should be done in this patch/series ;-) > > > > I tend to disagree here. Adding the direction as a lookup key > > is IMO beyond the scope of this patch. That's complicated and > > would defer this series by months. Given that the upcomming IPTFS > > implementation has a lot of direction specific config options, > > it makes sense to take that this patch now. Otherwise we have the > > direction specific options in input and output states forever. > I don't understand why the direction could not be mandatory and checked for new > options only (offload, iptfs, etc.) and reject for legacy use cases. Because every state has a direction and it should be marked explictly. As said, IMO it should have been like that from the beginning.
On Thu, Apr 11, 2024 at 09:14:15 +0200, Steffen Klassert wrote: > On Wed, Apr 10, 2024 at 08:32:20AM +0200, Nicolas Dichtel wrote: > > Le 09/04/2024 à 19:56, Antony Antony a écrit : > > > This patch introduces the 'dir' attribute, 'in' or 'out', to the > > > xfrm_state, SA, enhancing usability by delineating the scope of values > > > based on direction. An input SA will now exclusively encompass values > > > pertinent to input, effectively segregating them from output-related > > > values. This change aims to streamline the configuration process and > > > improve the overall clarity of SA attributes. > > > > > > This feature sets the groundwork for future patches, including > > > the upcoming IP-TFS patch. > > > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > > > --- > > > v8->v9: > > > - add validation XFRM_STATE_ICMP not allowed on OUT SA. > > > > > > v7->v8: > > > - add extra validation check on replay window and seq > > > - XFRM_MSG_UPDSA old and new SA should match "dir" > > > > > > v6->v7: > > > - add replay-window check non-esn 0 and ESN 1. > > > - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD > > Why? I still think that having an 'input' SA used in the output path is wrong > > and confusing. > > I don't think this can happen. This patch does not change the > state lookups, so we should match the correct state as it was > before that patch. > > On the long run, we should make the direction a lookup key. > That should have happened with the initial implemenatation > already, now ~25 years later we would have to maintain the > old input/output combined SADB and two new ones where input > and output states are separated. > +1 Talking about the history, the need for SA "dir" is long overdue. My issue is when offload was added they aded a new direction flag which is specific to off-load only. And now we want one for IP-TFS. Trying to restrict dir only to IP-TFS sounds bad idea. That is why I pushing for an information only SA "dir", and it not for look up at the moment. Any case, I will send v10, please wait. I think that address most concerns. We just have to polish the checks and error counter there. -antony
On Thu, Apr 11, 2024 at 11:04:21 +0200, Steffen Klassert wrote: > On Thu, Apr 11, 2024 at 11:01:32AM +0200, Nicolas Dichtel wrote: > > > > > > Le 11/04/2024 à 09:14, Steffen Klassert a écrit : > > > On Wed, Apr 10, 2024 at 08:32:20AM +0200, Nicolas Dichtel wrote: > > >> Le 09/04/2024 à 19:56, Antony Antony a écrit : > > >>> This patch introduces the 'dir' attribute, 'in' or 'out', to the > > >>> xfrm_state, SA, enhancing usability by delineating the scope of values > > >>> based on direction. An input SA will now exclusively encompass values > > >>> pertinent to input, effectively segregating them from output-related > > >>> values. This change aims to streamline the configuration process and > > >>> improve the overall clarity of SA attributes. > > >>> > > >>> This feature sets the groundwork for future patches, including > > >>> the upcoming IP-TFS patch. > > >>> > > >>> Signed-off-by: Antony Antony <antony.antony@secunet.com> > > >>> --- > > >>> v8->v9: > > >>> - add validation XFRM_STATE_ICMP not allowed on OUT SA. > > >>> > > >>> v7->v8: > > >>> - add extra validation check on replay window and seq > > >>> - XFRM_MSG_UPDSA old and new SA should match "dir" > > >>> > > >>> v6->v7: > > >>> - add replay-window check non-esn 0 and ESN 1. > > >>> - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD > > >> Why? I still think that having an 'input' SA used in the output path is wrong > > >> and confusing. > > > > > > I don't think this can happen. This patch does not change the > > > state lookups, so we should match the correct state as it was > > > before that patch. > > This is the point. The user can set whatever direction in the SA, there is no > > check. He can set the dir to 'output' even if the SA is used in input. > > Ah, yes indeed. That should be fixed, thanks for clarification! I have v10 with two simple packet path checks and error counters as Sabrina suggested. I imagine this would avoid inconsitancy Nicolas trying to avoid.
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 57c743b7e4fe..7c9be06f8302 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -291,6 +291,7 @@ struct xfrm_state { /* Private data of this transformer, format is opaque, * interpreted by xfrm_type methods. */ void *data; + u8 dir; }; static inline struct net *xs_net(struct xfrm_state *x) diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h index 6a77328be114..18ceaba8486e 100644 --- a/include/uapi/linux/xfrm.h +++ b/include/uapi/linux/xfrm.h @@ -141,6 +141,11 @@ enum { XFRM_POLICY_MAX = 3 }; +enum xfrm_sa_dir { + XFRM_SA_DIR_IN = 1, + XFRM_SA_DIR_OUT = 2 +}; + enum { XFRM_SHARE_ANY, /* No limitations */ XFRM_SHARE_SESSION, /* For this session only */ @@ -315,6 +320,7 @@ enum xfrm_attr_type_t { XFRMA_SET_MARK_MASK, /* __u32 */ XFRMA_IF_ID, /* __u32 */ XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */ + XFRMA_SA_DIR, /* __u8 */ __XFRMA_MAX #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */ diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c index 655fe4ff8621..007dee03b1bc 100644 --- a/net/xfrm/xfrm_compat.c +++ b/net/xfrm/xfrm_compat.c @@ -98,6 +98,7 @@ static const int compat_msg_min[XFRM_NR_MSGTYPES] = { }; static const struct nla_policy compat_policy[XFRMA_MAX+1] = { + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR }, [XFRMA_SA] = { .len = XMSGSIZE(compat_xfrm_usersa_info)}, [XFRMA_POLICY] = { .len = XMSGSIZE(compat_xfrm_userpolicy_info)}, [XFRMA_LASTUSED] = { .type = NLA_U64}, @@ -129,6 +130,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = { [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 }, [XFRMA_IF_ID] = { .type = NLA_U32 }, [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 }, + [XFRMA_SA_DIR] = { .type = NLA_U8} }; static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb, @@ -277,9 +279,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src) case XFRMA_SET_MARK_MASK: case XFRMA_IF_ID: case XFRMA_MTIMER_THRESH: + case XFRMA_SA_DIR: return xfrm_nla_cpy(dst, src, nla_len(src)); default: - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH); + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR); pr_warn_once("unsupported nla_type %d\n", src->nla_type); return -EOPNOTSUPP; } @@ -434,7 +437,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla, int err; if (type > XFRMA_MAX) { - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH); + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR); NL_SET_ERR_MSG(extack, "Bad attribute"); return -EOPNOTSUPP; } diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index 6346690d5c69..2455a76a1cff 100644 --- a/net/xfrm/xfrm_device.c +++ b/net/xfrm/xfrm_device.c @@ -253,6 +253,12 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x, return -EINVAL; } + if ((xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir == XFRM_SA_DIR_OUT) || + (!(xuo->flags & XFRM_OFFLOAD_INBOUND) && x->dir == XFRM_SA_DIR_IN)) { + NL_SET_ERR_MSG(extack, "Mismatched SA and offload direction"); + return -EINVAL; + } + is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET; /* We don't yet support UDP encapsulation and TFC padding. */ diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 0c306473a79d..f7771a69ae2e 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, x->lastused = orig->lastused; x->new_mapping = 0; x->new_mapping_sport = 0; + x->dir = orig->dir; return x; @@ -1857,6 +1858,9 @@ int xfrm_state_update(struct xfrm_state *x) if (!x1) goto out; + if (x1->dir != x->dir) + goto out; + if (xfrm_state_kern(x1)) { to_put = x1; err = -EEXIST; diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 810b520493f3..9c1857f95058 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -360,6 +360,62 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, } } + if (attrs[XFRMA_SA_DIR]) { + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]); + + if (sa_dir != XFRM_SA_DIR_IN && sa_dir != XFRM_SA_DIR_OUT) { + NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range"); + err = -EINVAL; + goto out; + } + + if (sa_dir == XFRM_SA_DIR_OUT) { + if (p->replay_window > 0) { + NL_SET_ERR_MSG(extack, "Replay window should not be set for OUT SA"); + err = -EINVAL; + goto out; + } + + if (attrs[XFRMA_REPLAY_VAL]) { + struct xfrm_replay_state *rp; + + rp = nla_data(attrs[XFRMA_REPLAY_VAL]); + if (rp->oseq || rp->seq || rp->bitmap) { + NL_SET_ERR_MSG(extack, + "Replay seq, oseq, or bitmap should not be set for OUT SA with ESN"); + err = -EINVAL; + goto out; + } + } + + if (attrs[XFRMA_REPLAY_ESN_VAL]) { + struct xfrm_replay_state_esn *esn; + + esn = nla_data(attrs[XFRMA_REPLAY_ESN_VAL]); + if (esn->replay_window > 1) { + NL_SET_ERR_MSG(extack, + "Replay window should be 1 for OUT SA with ESN"); + err = -EINVAL; + goto out; + } + + if (esn->seq || esn->seq_hi || esn->bmp_len) { + NL_SET_ERR_MSG(extack, + "Replay seq, seq_hi, bmp_len should not be set for OUT SA with ESN"); + err = -EINVAL; + goto out; + } + } + + if (p->flags & XFRM_STATE_ICMP) { + NL_SET_ERR_MSG(extack, + "Flag ICMP not be set for OUT SA"); + err = -EINVAL; + goto out; + } + } + } + out: return err; } @@ -627,6 +683,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs, struct nlattr *et = attrs[XFRMA_ETIMER_THRESH]; struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH]; struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH]; + struct nlattr *dir = attrs[XFRMA_SA_DIR]; if (re && x->replay_esn && x->preplay_esn) { struct xfrm_replay_state_esn *replay_esn; @@ -661,6 +718,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs, if (mt) x->mapping_maxage = nla_get_u32(mt); + + if (dir) + x->dir = nla_get_u8(dir); } static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m) @@ -1182,8 +1242,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x, if (ret) goto out; } - if (x->mapping_maxage) + if (x->mapping_maxage) { ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage); + if (ret) + goto out; + } + if (x->dir) + ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); out: return ret; } @@ -2402,7 +2467,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x) + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur)) + nla_total_size(sizeof(struct xfrm_mark)) + nla_total_size(4) /* XFRM_AE_RTHR */ - + nla_total_size(4); /* XFRM_AE_ETHR */ + + nla_total_size(4) /* XFRM_AE_ETHR */ + + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */ } static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c) @@ -2459,6 +2525,12 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct if (err) goto out_cancel; + if (x->dir) { + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); + if (err) + goto out_cancel; + } + nlmsg_end(skb, nlh); return 0; @@ -3018,6 +3090,7 @@ EXPORT_SYMBOL_GPL(xfrm_msg_min); #undef XMSGSIZE const struct nla_policy xfrma_policy[XFRMA_MAX+1] = { + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR }, [XFRMA_SA] = { .len = sizeof(struct xfrm_usersa_info)}, [XFRMA_POLICY] = { .len = sizeof(struct xfrm_userpolicy_info)}, [XFRMA_LASTUSED] = { .type = NLA_U64}, @@ -3049,6 +3122,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = { [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 }, [XFRMA_IF_ID] = { .type = NLA_U32 }, [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 }, + [XFRMA_SA_DIR] = { .type = NLA_U8 } }; EXPORT_SYMBOL_GPL(xfrma_policy); @@ -3189,8 +3263,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb) static inline unsigned int xfrm_expire_msgsize(void) { - return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) - + nla_total_size(sizeof(struct xfrm_mark)); + return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) + + nla_total_size(sizeof(struct xfrm_mark)) + + nla_total_size(sizeof_field(struct xfrm_state, dir)); } static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c) @@ -3217,6 +3292,12 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct if (err) return err; + if (x->dir) { + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir); + if (err) + return err; + } + nlmsg_end(skb, nlh); return 0; } @@ -3324,6 +3405,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x) if (x->mapping_maxage) l += nla_total_size(sizeof(x->mapping_maxage)); + if (x->dir) + l += nla_total_size(sizeof(x->dir)); + return l; }
This patch introduces the 'dir' attribute, 'in' or 'out', to the xfrm_state, SA, enhancing usability by delineating the scope of values based on direction. An input SA will now exclusively encompass values pertinent to input, effectively segregating them from output-related values. This change aims to streamline the configuration process and improve the overall clarity of SA attributes. This feature sets the groundwork for future patches, including the upcoming IP-TFS patch. Signed-off-by: Antony Antony <antony.antony@secunet.com> --- v8->v9: - add validation XFRM_STATE_ICMP not allowed on OUT SA. v7->v8: - add extra validation check on replay window and seq - XFRM_MSG_UPDSA old and new SA should match "dir" v6->v7: - add replay-window check non-esn 0 and ESN 1. - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD v5->v6: - XFRMA_SA_DIR only allowed with HW OFFLOAD v4->v5: - add details to commit message v3->v4: - improve HW OFFLOAD DIR check add the other direction v2->v3: - delete redundant XFRM_SA_DIR_USE - use u8 for "dir" - fix HW OFFLOAD DIR check v1->v2: - use .strict_start_type in struct nla_policy xfrma_policy - delete redundant XFRM_SA_DIR_MAX enum --- include/net/xfrm.h | 1 + include/uapi/linux/xfrm.h | 6 +++ net/xfrm/xfrm_compat.c | 7 ++- net/xfrm/xfrm_device.c | 6 +++ net/xfrm/xfrm_state.c | 4 ++ net/xfrm/xfrm_user.c | 92 +++++++++++++++++++++++++++++++++++++-- 6 files changed, 110 insertions(+), 6 deletions(-) -- 2.30.2