Message ID | 20221031152612.o3h44x3whath4iyp@intra2net.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | xfrm: Fix oops in __xfrm_state_delete() | expand |
On Mon, Oct 31, 2022 at 04:26:12PM +0100, Thomas Jarosch wrote: > > diff --git a/net/key/af_key.c b/net/key/af_key.c > index c85df5b958d2..65a9ede62d65 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -3382,7 +3382,7 @@ static int pfkey_send_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, > hdr->sadb_msg_len = size / sizeof(uint64_t); > hdr->sadb_msg_errno = 0; > hdr->sadb_msg_reserved = 0; > - hdr->sadb_msg_seq = x->km.seq = get_acqseq(); > + hdr->sadb_msg_seq = get_acqseq(); This looks broken. x->km.seq is part of the state which you are changing. Shouldn't you do whatever xfrm_user does in the same situation? Thanks,
On Tue, Nov 01, 2022 at 12:39:48 +0800, Herbert Xu wrote: > On Mon, Oct 31, 2022 at 04:26:12PM +0100, Thomas Jarosch wrote: > > > > diff --git a/net/key/af_key.c b/net/key/af_key.c > > index c85df5b958d2..65a9ede62d65 100644 > > --- a/net/key/af_key.c > > +++ b/net/key/af_key.c > > @@ -3382,7 +3382,7 @@ static int pfkey_send_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, > > hdr->sadb_msg_len = size / sizeof(uint64_t); > > hdr->sadb_msg_errno = 0; > > hdr->sadb_msg_reserved = 0; > > - hdr->sadb_msg_seq = x->km.seq = get_acqseq(); This line looks very odd. > > + hdr->sadb_msg_seq = get_acqseq(); > > This looks broken. x->km.seq is part of the state which you are > changing. Shouldn't you do whatever xfrm_user does in the same > situation? xfrm_user sets msg_seq to zero in mapping change message. seq is only useful for acquire message. I think setting to zero would be a better fix. - hdr->sadb_msg_seq = x->km.seq = get_acqseq(); + hdr->sadb_msg_seq = 0; While increasing x->km.seq in every call to pfkey_send_new_mapping() could be an issue, would it alone explan the crash? Tobias would pfkey_send_new_mapping() called in a default setting?
On Tue, Nov 01, 2022 at 08:10:21PM +0100, Antony Antony wrote: > > xfrm_user sets msg_seq to zero in mapping change message. seq is only useful for Oh I had misread the patch and thought this was send_acquire. > acquire message. I think setting to zero would be a better fix. > > - hdr->sadb_msg_seq = x->km.seq = get_acqseq(); > + hdr->sadb_msg_seq = 0; > > While increasing x->km.seq in every call to pfkey_send_new_mapping() > could be an issue, would it alone explan the crash? Probably, if you change the state without moving it to the right hash slot then the xfrm state hash table will be inconsistent. We should copy the xfrm_user behaviour which is to leave x->km.seq alone. So the patch should change the above line to hdr->sadb_msg_seq = x->km.seq; Thanks,
Hi Herbert, You wrote on Wed, Nov 02, 2022 at 03:07:57PM +0800: > > xfrm_user sets msg_seq to zero in mapping change message. seq is only useful for > > Oh I had misread the patch and thought this was send_acquire. it's a complex bug after all ^^ it took many printks() to trace the flow of the state corruption on the production system. > > acquire message. I think setting to zero would be a better fix. > > > > - hdr->sadb_msg_seq = x->km.seq = get_acqseq(); > > + hdr->sadb_msg_seq = 0; > > > > While increasing x->km.seq in every call to pfkey_send_new_mapping() > > could be an issue, would it alone explan the crash? > > Probably, if you change the state without moving it to the right > hash slot then the xfrm state hash table will be inconsistent. in the observed cases, km.seq is always zero before. So it was never added to the byseq hash table in the first place, resulting in the NULL pointer Oops. If km.seq would be non-zero before entering pfkey_send_new_mapping(), then of course the xfrm_state would stay in the wrong hash table bucket. The only other xfrm_states I've seen in my extensive tracing with a non-zero sequence number were ACQUIRE states and I'm not sure those will ever end up on the pfkey_send_new_mapping() code path. Either way, let's fix the root cause. > We should copy the xfrm_user behaviour which is to leave x->km.seq > alone. So the patch should change the above line to > > hdr->sadb_msg_seq = x->km.seq; thanks for your feedback, I'll update the patch and send a v2. I'll also put it in production tonight. Cheers, Thomas
diff --git a/net/key/af_key.c b/net/key/af_key.c index c85df5b958d2..65a9ede62d65 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c @@ -3382,7 +3382,7 @@ static int pfkey_send_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, hdr->sadb_msg_len = size / sizeof(uint64_t); hdr->sadb_msg_errno = 0; hdr->sadb_msg_reserved = 0; - hdr->sadb_msg_seq = x->km.seq = get_acqseq(); + hdr->sadb_msg_seq = get_acqseq(); hdr->sadb_msg_pid = 0; /* SA */