| Message ID | fe554921-104e-2365-a09b-812a1cedac65@secunet.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-ipsec,v2] xfrm: replay: Fix ESN wrap around for GSO | expand |
On Tue, 27 Sep 2022 14:59:50 +0200 Christian Langrock wrote: > When using GSO it can happen that the wrong seq_hi is used for the last > packets before the wrap around. This can lead to double usage of a > sequence number. To avoid this, we should serialize this last GSO > packet. Does not build but please wait for reviews before reposting: net/xfrm/xfrm_replay.c:773:6: error: conflicting types for ‘xfrm_replay_overflow_check’; have ‘bool(struct xfrm_state *, struct sk_buff *)’ {aka ‘_Bool(struct xfrm_state *, struct sk_buff *)’} 773 | bool xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb) | ^~~~~~~~~~~~~~~~~~~~~~~~~~
On Tue, Sep 27, 2022 at 02:59:50PM +0200, Christian Langrock wrote: > When using GSO it can happen that the wrong seq_hi is used for the last > packets before the wrap around. This can lead to double usage of a > sequence number. To avoid this, we should serialize this last GSO > packet. > > Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for...") > Signed-off-by: Christian Langrock <christian.langrock@secunet.com> > --- > include/net/xfrm.h | 1 + > net/xfrm/xfrm_output.c | 2 +- > net/xfrm/xfrm_replay.c | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > index 6e8fa98f786f..49d6d974f493 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -1749,6 +1749,7 @@ void xfrm_replay_advance(struct xfrm_state *x, __be32 net_seq); > int xfrm_replay_check(struct xfrm_state *x, struct sk_buff *skb, __be32 net_seq); > void xfrm_replay_notify(struct xfrm_state *x, int event); > int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb); > +int xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb); > int xfrm_replay_recheck(struct xfrm_state *x, struct sk_buff *skb, __be32 net_seq); > > static inline int xfrm_aevent_is_on(struct net *net) > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c > index 9a5e79a38c67..c470a68d9c88 100644 > --- a/net/xfrm/xfrm_output.c > +++ b/net/xfrm/xfrm_output.c > @@ -738,7 +738,7 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb) > skb->encapsulation = 1; > > if (skb_is_gso(skb)) { > - if (skb->inner_protocol) > + if (skb->inner_protocol || xfrm_replay_overflow_check(x, skb)) > return xfrm_output_gso(net, sk, skb); > > skb_shinfo(skb)->gso_type |= SKB_GSO_ESP; > diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c > index 9277d81b344c..991cfc7a091d 100644 > --- a/net/xfrm/xfrm_replay.c > +++ b/net/xfrm/xfrm_replay.c > @@ -750,6 +750,34 @@ int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb) > > return xfrm_replay_overflow_offload(x, skb); > } > + > +static bool xfrm_replay_overflow_check_offload_esn(struct xfrm_state *x, struct sk_buff *skb) > +{ > + struct xfrm_replay_state_esn *replay_esn = x->replay_esn; > + __u32 oseq = replay_esn->oseq; > + bool ret = false; > + > + /* We assume that this function is called with > + * skb_is_gso(skb) == true > + */ > + > + if (x->type->flags & XFRM_TYPE_REPLAY_PROT) { > + oseq = oseq + 1 + skb_shinfo(skb)->gso_segs; > + if (unlikely(oseq < replay_esn->oseq)) > + ret = true; return true; > + } > + > + return ret; return false; > +} > + > +bool xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb) > +{ > + if (x->repl_mode == XFRM_REPLAY_MODE_ESN) > + return xfrm_replay_overflow_check_offload_esn(x, skb); > + > + return false; > +} I still think that functions above should be merged into one. This is called only if xfrm_dev_offload_ok() success -> in crypto offload path. Thanks > + > #else > int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb) > { > @@ -764,6 +792,11 @@ int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb) > > return __xfrm_replay_overflow(x, skb); > } > + > +int xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb) int -> bool > +{ > + return 0; > +} > #endif > > int xfrm_init_replay(struct xfrm_state *x) > -- > 2.37.1.223.g6a475b71f8 >
On Tue, Sep 27, 2022 at 02:59:50PM +0200, Christian Langrock wrote: > When using GSO it can happen that the wrong seq_hi is used for the last > packets before the wrap around. This can lead to double usage of a > sequence number. To avoid this, we should serialize this last GSO > packet. > > Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for...") > Signed-off-by: Christian Langrock <christian.langrock@secunet.com> Some minor nits: This is already v3, not v2 as stated in the subject line. Also, please explain the changes between the versions (see 'git log' for examples). The target tree is 'ipsec', not 'net-ipsec'. Otherwise this is a fix for a real bug. So fix the build, incorporate the review from Leon and send a v4. Thanks!
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on klassert-ipsec/master]
[also build test ERROR on net-next/master net/master linus/master v6.0-rc7 next-20220927]
[cannot apply to klassert-ipsec-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Langrock/xfrm-replay-Fix-ESN-wrap-around-for-GSO/20220927-210053
base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
config: i386-randconfig-a013
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/bb1661466e741ea02b82c9d57c6cee6cee07ba7e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Langrock/xfrm-replay-Fix-ESN-wrap-around-for-GSO/20220927-210053
git checkout bb1661466e741ea02b82c9d57c6cee6cee07ba7e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> net/xfrm/xfrm_replay.c:773:6: error: conflicting types for 'xfrm_replay_overflow_check'
bool xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb)
^
include/net/xfrm.h:1752:5: note: previous declaration is here
int xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb);
^
1 error generated.
vim +/xfrm_replay_overflow_check +773 net/xfrm/xfrm_replay.c
772
> 773 bool xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb)
774 {
775 if (x->repl_mode == XFRM_REPLAY_MODE_ESN)
776 return xfrm_replay_overflow_check_offload_esn(x, skb);
777
778 return false;
779 }
780
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on klassert-ipsec/master]
[also build test ERROR on net-next/master net/master linus/master v6.0-rc7 next-20220927]
[cannot apply to klassert-ipsec-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Langrock/xfrm-replay-Fix-ESN-wrap-around-for-GSO/20220927-210053
base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
config: x86_64-rhel-8.3-func
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/bb1661466e741ea02b82c9d57c6cee6cee07ba7e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Langrock/xfrm-replay-Fix-ESN-wrap-around-for-GSO/20220927-210053
git checkout bb1661466e741ea02b82c9d57c6cee6cee07ba7e
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> net/xfrm/xfrm_replay.c:773:6: error: conflicting types for 'xfrm_replay_overflow_check'; have 'bool(struct xfrm_state *, struct sk_buff *)' {aka '_Bool(struct xfrm_state *, struct sk_buff *)'}
773 | bool xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from net/xfrm/xfrm_replay.c:10:
include/net/xfrm.h:1752:5: note: previous declaration of 'xfrm_replay_overflow_check' with type 'int(struct xfrm_state *, struct sk_buff *)'
1752 | int xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +773 net/xfrm/xfrm_replay.c
772
> 773 bool xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb)
774 {
775 if (x->repl_mode == XFRM_REPLAY_MODE_ESN)
776 return xfrm_replay_overflow_check_offload_esn(x, skb);
777
778 return false;
779 }
780
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 6e8fa98f786f..49d6d974f493 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1749,6 +1749,7 @@ void xfrm_replay_advance(struct xfrm_state *x, __be32 net_seq); int xfrm_replay_check(struct xfrm_state *x, struct sk_buff *skb, __be32 net_seq); void xfrm_replay_notify(struct xfrm_state *x, int event); int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb); +int xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb); int xfrm_replay_recheck(struct xfrm_state *x, struct sk_buff *skb, __be32 net_seq); static inline int xfrm_aevent_is_on(struct net *net) diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c index 9a5e79a38c67..c470a68d9c88 100644 --- a/net/xfrm/xfrm_output.c +++ b/net/xfrm/xfrm_output.c @@ -738,7 +738,7 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb) skb->encapsulation = 1; if (skb_is_gso(skb)) { - if (skb->inner_protocol) + if (skb->inner_protocol || xfrm_replay_overflow_check(x, skb)) return xfrm_output_gso(net, sk, skb); skb_shinfo(skb)->gso_type |= SKB_GSO_ESP; diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c index 9277d81b344c..991cfc7a091d 100644 --- a/net/xfrm/xfrm_replay.c +++ b/net/xfrm/xfrm_replay.c @@ -750,6 +750,34 @@ int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb) return xfrm_replay_overflow_offload(x, skb); } + +static bool xfrm_replay_overflow_check_offload_esn(struct xfrm_state *x, struct sk_buff *skb) +{ + struct xfrm_replay_state_esn *replay_esn = x->replay_esn; + __u32 oseq = replay_esn->oseq; + bool ret = false; + + /* We assume that this function is called with + * skb_is_gso(skb) == true + */ + + if (x->type->flags & XFRM_TYPE_REPLAY_PROT) { + oseq = oseq + 1 + skb_shinfo(skb)->gso_segs; + if (unlikely(oseq < replay_esn->oseq)) + ret = true; + } + + return ret; +} + +bool xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb) +{ + if (x->repl_mode == XFRM_REPLAY_MODE_ESN) + return xfrm_replay_overflow_check_offload_esn(x, skb); + + return false; +} + #else int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb) { @@ -764,6 +792,11 @@ int xfrm_replay_overflow(struct xfrm_state *x, struct sk_buff *skb) return __xfrm_replay_overflow(x, skb); } + +int xfrm_replay_overflow_check(struct xfrm_state *x, struct sk_buff *skb) +{ + return 0; +} #endif int xfrm_init_replay(struct xfrm_state *x)
When using GSO it can happen that the wrong seq_hi is used for the last packets before the wrap around. This can lead to double usage of a sequence number. To avoid this, we should serialize this last GSO packet. Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for...") Signed-off-by: Christian Langrock <christian.langrock@secunet.com> --- include/net/xfrm.h | 1 + net/xfrm/xfrm_output.c | 2 +- net/xfrm/xfrm_replay.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-)