ok mvs@
> On 13 May 2021, at 02:43, YASUOKA Masahiko <yasu...@openbsd.org> wrote: > > On Wed, 12 May 2021 19:15:29 +0300 > Vitaliy Makkoveev <m...@openbsd.org> wrote: >>> On 12 May 2021, at 18:42, YASUOKA Masahiko <yasu...@openbsd.org> wrote: >>> On Wed, 12 May 2021 17:26:51 +0300 >>> Vitaliy Makkoveev <m...@openbsd.org> wrote: >>>> On Wed, May 12, 2021 at 07:11:09PM +0900, YASUOKA Masahiko wrote: >>>>> Radek reported a problem to misc@ that multiple Windows clients behind a >>>>> NAT >>>>> cannot use a L2TP/IPsec server simultaneously. >>>>> >>>>> https://marc.info/?t=160996816100001&r=1&w=2 >>>>> >>>>> There is two problems. First is pipex(4) doesn't pass the proper >>>>> ipsecflowinfo to ip_output(). Second is the IPsec policy check which is >>>>> done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is not >>>>> cached. This happens when its flow is shared by another tdb (for another >>>>> client of the same NAT). >>>>> >>>>> The following 2 diffs fix these problem. >>>>> >>>>> comment? >>>>> ok? >>>>> >>>> >>>> Hi. >>>> >>>> I have two comments for the diff 1: >>>> >>>> 1. You should add PACKET_TAG_IPSEC_FLOWINFO description to >>>> m_tag_get(9). >>>> 2. You introduced mbuf(9) leak in pipex_l2tp_output() error path. I >>>> pointed the place in your diff. >>> >>> Good catch. Thanks. >>> >> >> m_freem(9) accepts NULL so this check before is redundant. > > Yes, > >> It seems to me that "Used by the IPv4 stack to specify the IPsec flow >> of an output IP packet. The tag contains a u_int32_t identifying the >> IPsec flow.” is enough. Anyway it’s better to ask jmc@. > > Ok, > >> Also I like to remove PACKET_TAG_PIPEX with separate diff. > > I removed PACKET_TAG_PIPEX separetely. > > Let me update the diff. > > Index: sys/net/pipex.c > =================================================================== > RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v > retrieving revision 1.132 > diff -u -p -r1.132 pipex.c > --- sys/net/pipex.c 10 Mar 2021 10:21:48 -0000 1.132 > +++ sys/net/pipex.c 12 May 2021 23:18:52 -0000 > @@ -1628,6 +1628,7 @@ pipex_l2tp_output(struct mbuf *m0, struc > #ifdef INET6 > struct ip6_hdr *ip6; > #endif > + struct m_tag *mtag; > > hlen = sizeof(struct pipex_l2tp_header) + > ((pipex_session_is_l2tp_data_sequencing_on(session)) > @@ -1704,6 +1705,15 @@ pipex_l2tp_output(struct mbuf *m0, struc > ip->ip_tos = 0; > ip->ip_off = 0; > > + if (session->proto.l2tp.ipsecflowinfo > 0) { > + if ((mtag = m_tag_get(PACKET_TAG_IPSEC_FLOWINFO, > + sizeof(u_int32_t), M_NOWAIT)) == NULL) > + goto drop; > + *(u_int32_t *)(mtag + 1) = > + session->proto.l2tp.ipsecflowinfo; > + m_tag_prepend(m0, mtag); > + } > + > ip_send(m0); > break; > #ifdef INET6 > @@ -1733,6 +1743,7 @@ pipex_l2tp_output(struct mbuf *m0, struc > > return; > drop: > + m_freem(m0); > session->stat.oerrors++; > } > > Index: sys/netinet/ip_input.c > =================================================================== > RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v > retrieving revision 1.359 > diff -u -p -r1.359 ip_input.c > --- sys/netinet/ip_input.c 30 Apr 2021 13:52:48 -0000 1.359 > +++ sys/netinet/ip_input.c 12 May 2021 23:18:52 -0000 > @@ -1790,6 +1790,8 @@ ip_send_do_dispatch(void *xmq, int flags > struct mbuf_queue *mq = xmq; > struct mbuf *m; > struct mbuf_list ml; > + struct m_tag *mtag; > + u_int32_t ipsecflowinfo = 0; > > mq_delist(mq, &ml); > if (ml_empty(&ml)) > @@ -1797,7 +1799,12 @@ ip_send_do_dispatch(void *xmq, int flags > > NET_LOCK(); > while ((m = ml_dequeue(&ml)) != NULL) { > - ip_output(m, NULL, NULL, flags, NULL, NULL, 0); > + if ((mtag = m_tag_find(m, PACKET_TAG_IPSEC_FLOWINFO, NULL)) > + != NULL) { > + ipsecflowinfo = *(u_int32_t *)(mtag + 1); > + m_tag_delete(m, mtag); > + } > + ip_output(m, NULL, NULL, flags, NULL, NULL, ipsecflowinfo); > } > NET_UNLOCK(); > } > Index: sys/sys/mbuf.h > =================================================================== > RCS file: /disk/cvs/openbsd/src/sys/sys/mbuf.h,v > retrieving revision 1.252 > diff -u -p -r1.252 mbuf.h > --- sys/sys/mbuf.h 25 Feb 2021 02:43:31 -0000 1.252 > +++ sys/sys/mbuf.h 12 May 2021 23:18:52 -0000 > @@ -469,6 +469,7 @@ struct m_tag *m_tag_next(struct mbuf *, > /* Packet tag types */ > #define PACKET_TAG_IPSEC_IN_DONE 0x0001 /* IPsec applied, in */ > #define PACKET_TAG_IPSEC_OUT_DONE 0x0002 /* IPsec applied, out */ > +#define PACKET_TAG_IPSEC_FLOWINFO 0x0004 /* IPsec flowinfo */ > #define PACKET_TAG_WIREGUARD 0x0040 /* WireGuard data */ > #define PACKET_TAG_GRE 0x0080 /* GRE processing done > */ > #define PACKET_TAG_DLT 0x0100 /* data link layer type > */ > @@ -479,7 +480,7 @@ struct m_tag *m_tag_next(struct mbuf *, > #define PACKET_TAG_CARP_BAL_IP 0x4000 /* carp(4) ip balanced > marker */ > > #define MTAG_BITS \ > - ("\20\1IPSEC_IN_DONE\2IPSEC_OUT_DONE\3IPSEC_IN_CRYPTO_DONE" \ > + ("\20\1IPSEC_IN_DONE\2IPSEC_OUT_DONE\3IPSEC_FLOWINFO" \ > "\4IPSEC_OUT_CRYPTO_NEEDED\5IPSEC_PENDING_TDB\6BRIDGE\7WG\10GRE\11DLT" \ > "\12PF_DIVERT\14PF_REASSEMBLED\15SRCROUTE\16TUNNEL\17CARP_BAL_IP") > > Index: share/man/man9/mbuf_tags.9 > =================================================================== > RCS file: /disk/cvs/openbsd/src/share/man/man9/mbuf_tags.9,v > retrieving revision 1.41 > diff -u -p -r1.41 mbuf_tags.9 > --- share/man/man9/mbuf_tags.9 21 Jun 2020 15:25:30 -0000 1.41 > +++ share/man/man9/mbuf_tags.9 12 May 2021 23:18:52 -0000 > @@ -122,6 +122,11 @@ The tag contains a > identifying the security association applied to the packet. > This tag is primarily used to detect and avoid loops in IPsec > processing on output. > +.It PACKET_TAG_IPSEC_FLOWINFO > +Used by the IPv4 stack to specify the IPsec flow of an output IP packet. > +The tag contains a > +.Va u_int32_t > +identifying the IPsec flow. > .It PACKET_TAG_WIREGUARD > Used by the > .Xr wg 4