Re: pfkey vs splsoftnet()
On 12.1.2017. 18:27, Hrvoje Popovski wrote: > On 12.1.2017. 16:20, Martin Pieuchot wrote: >> On 10/01/17(Tue) 10:37, Martin Pieuchot wrote: >>> In pfkey_sendup() we call sorwakeup() which asserts for NET_LOCK(), so >>> we are already at IPL_SOFTNET. >>> >>> pfkeyv2_send() is called via pfkey_output() which is also called with >>> the NET_LOCK() held. >>> >>> Finally replace a comment above pfkeyv2_ipo_walk() by the corresponding >>> assert. >> Anybody tested or reviewed this diff? >> > > > I applied this diff in production box with > > carp > pf > pfsync > isakmpd -K4 > sasyncd > dhcpd > dhcpd sync > tcpdump -lnqttti pflog0 and pflow ipfix and still solid as rock :)
Re: pfkey vs splsoftnet()
On Tue, Jan 10, 2017 at 10:37:38AM +0100, Martin Pieuchot wrote:
> In pfkey_sendup() we call sorwakeup() which asserts for NET_LOCK(), so
> we are already at IPL_SOFTNET.
>
> pfkeyv2_send() is called via pfkey_output() which is also called with
> the NET_LOCK() held.
>
> Finally replace a comment above pfkeyv2_ipo_walk() by the corresponding
> assert.
>
> ok?
OK bluhm@
>
> Index: net/pfkey.c
> ===
> RCS file: /cvs/src/sys/net/pfkey.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 pfkey.c
> --- net/pfkey.c 29 Nov 2016 10:22:30 - 1.33
> +++ net/pfkey.c 10 Jan 2017 09:29:59 -
> @@ -136,7 +136,8 @@ int
> pfkey_sendup(struct socket *socket, struct mbuf *packet, int more)
> {
> struct mbuf *packet2;
> - int s;
> +
> + splsoftassert(IPL_SOFTNET);
>
> if (more) {
> if (!(packet2 = m_dup_pkt(packet, 0, M_DONTWAIT)))
> @@ -144,13 +145,10 @@ pfkey_sendup(struct socket *socket, stru
> } else
> packet2 = packet;
>
> - s = splsoftnet();
> if (!sbappendaddr(&socket->so_rcv, &pfkey_addr, packet2, NULL)) {
> m_freem(packet2);
> - splx(s);
> return (ENOBUFS);
> }
> - splx(s);
>
> sorwakeup(socket);
> return (0);
> Index: net/pfkeyv2.c
> ===
> RCS file: /cvs/src/sys/net/pfkeyv2.c,v
> retrieving revision 1.148
> diff -u -p -r1.148 pfkeyv2.c
> --- net/pfkeyv2.c 15 Sep 2016 02:00:18 - 1.148
> +++ net/pfkeyv2.c 10 Jan 2017 09:29:41 -
> @@ -783,7 +783,7 @@ int
> pfkeyv2_send(struct socket *socket, void *message, int len)
> {
> int i, j, rval = 0, mode = PFKEYV2_SENDMESSAGE_BROADCAST;
> - int delflag = 0, s;
> + int delflag = 0;
> struct sockaddr_encap encapdst, encapnetmask;
> struct ipsec_policy *ipo, *tmpipo;
> struct ipsec_acquire *ipa;
> @@ -807,6 +807,8 @@ pfkeyv2_send(struct socket *socket, void
>
> u_int rdomain;
>
> + splsoftassert(IPL_SOFTNET);
> +
> /* Verify that we received this over a legitimate pfkeyv2 socket */
> bzero(headers, sizeof(headers));
>
> @@ -940,8 +942,6 @@ pfkeyv2_send(struct socket *socket, void
> }
> #endif /* IPSEC */
>
> - s = splsoftnet();
> -
> /* Find TDB */
> sa2 = gettdb(rdomain, ssa->sadb_sa_spi, sunionp,
> SADB_X_GETSPROTO(smsg->sadb_msg_satype));
> @@ -949,7 +949,7 @@ pfkeyv2_send(struct socket *socket, void
> /* If there's no such SA, we're done */
> if (sa2 == NULL) {
> rval = ESRCH;
> - goto splxret;
> + goto ret;
> }
>
> /* If this is a reserved SA */
> @@ -969,7 +969,7 @@ pfkeyv2_send(struct socket *socket, void
> &newsa->tdb_sproto, &alg))) {
> tdb_free(freeme);
> freeme = NULL;
> - goto splxret;
> + goto ret;
> }
>
> /* Initialize SA */
> @@ -1020,7 +1020,7 @@ pfkeyv2_send(struct socket *socket, void
> rval = EINVAL;
> tdb_free(freeme);
> freeme = NULL;
> - goto splxret;
> + goto ret;
> }
>
> newsa->tdb_cur_allocations = sa2->tdb_cur_allocations;
> @@ -1041,7 +1041,7 @@ pfkeyv2_send(struct socket *socket, void
> headers[SADB_EXT_IDENTITY_DST] ||
> headers[SADB_EXT_SENSITIVITY]) {
> rval = EINVAL;
> - goto splxret;
> + goto ret;
> }
>
> import_sa(sa2, headers[SADB_EXT_SA], NULL);
> @@ -1059,7 +1059,6 @@ pfkeyv2_send(struct socket *socket, void
> #endif
> }
>
> - splx(s);
> break;
> case SADB_ADD:
> ssa = (struct sadb_sa *) headers[SADB_EXT_SA];
> @@ -1092,21 +1091,19 @@ pfkeyv2_send(struct socket *socket, void
> }
> #endif /* IPSEC */
>
> - s = splsoftnet();
> -
> sa2 = gettdb(rdomain, ssa->sadb_sa_spi, sunionp,
> SADB_X_GETSPROTO(smsg->sadb_msg_satype));
>
> /* We can't add an existing SA! */
> if (sa2 != NULL) {
> rval = EEXIST;
> - goto splxret;
> + goto ret;
> }
>
> /* We can only add "mature" SAs */
> if (ssa->sadb_sa_state != SADB_SASTATE_MATURE) {
> rval = EINVAL;
> -
Re: pfkey vs splsoftnet()
On 12.1.2017. 16:20, Martin Pieuchot wrote: > On 10/01/17(Tue) 10:37, Martin Pieuchot wrote: >> In pfkey_sendup() we call sorwakeup() which asserts for NET_LOCK(), so >> we are already at IPL_SOFTNET. >> >> pfkeyv2_send() is called via pfkey_output() which is also called with >> the NET_LOCK() held. >> >> Finally replace a comment above pfkeyv2_ipo_walk() by the corresponding >> assert. > Anybody tested or reviewed this diff? > I applied this diff in production box with carp pf pfsync isakmpd -K4 sasyncd dhcpd dhcpd sync tcpdump -lnqttti pflog0 and everything seems fine ...
Re: pfkey vs splsoftnet()
On 10/01/17(Tue) 10:37, Martin Pieuchot wrote:
> In pfkey_sendup() we call sorwakeup() which asserts for NET_LOCK(), so
> we are already at IPL_SOFTNET.
>
> pfkeyv2_send() is called via pfkey_output() which is also called with
> the NET_LOCK() held.
>
> Finally replace a comment above pfkeyv2_ipo_walk() by the corresponding
> assert.
Anybody tested or reviewed this diff?
> Index: net/pfkey.c
> ===
> RCS file: /cvs/src/sys/net/pfkey.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 pfkey.c
> --- net/pfkey.c 29 Nov 2016 10:22:30 - 1.33
> +++ net/pfkey.c 10 Jan 2017 09:29:59 -
> @@ -136,7 +136,8 @@ int
> pfkey_sendup(struct socket *socket, struct mbuf *packet, int more)
> {
> struct mbuf *packet2;
> - int s;
> +
> + splsoftassert(IPL_SOFTNET);
>
> if (more) {
> if (!(packet2 = m_dup_pkt(packet, 0, M_DONTWAIT)))
> @@ -144,13 +145,10 @@ pfkey_sendup(struct socket *socket, stru
> } else
> packet2 = packet;
>
> - s = splsoftnet();
> if (!sbappendaddr(&socket->so_rcv, &pfkey_addr, packet2, NULL)) {
> m_freem(packet2);
> - splx(s);
> return (ENOBUFS);
> }
> - splx(s);
>
> sorwakeup(socket);
> return (0);
> Index: net/pfkeyv2.c
> ===
> RCS file: /cvs/src/sys/net/pfkeyv2.c,v
> retrieving revision 1.148
> diff -u -p -r1.148 pfkeyv2.c
> --- net/pfkeyv2.c 15 Sep 2016 02:00:18 - 1.148
> +++ net/pfkeyv2.c 10 Jan 2017 09:29:41 -
> @@ -783,7 +783,7 @@ int
> pfkeyv2_send(struct socket *socket, void *message, int len)
> {
> int i, j, rval = 0, mode = PFKEYV2_SENDMESSAGE_BROADCAST;
> - int delflag = 0, s;
> + int delflag = 0;
> struct sockaddr_encap encapdst, encapnetmask;
> struct ipsec_policy *ipo, *tmpipo;
> struct ipsec_acquire *ipa;
> @@ -807,6 +807,8 @@ pfkeyv2_send(struct socket *socket, void
>
> u_int rdomain;
>
> + splsoftassert(IPL_SOFTNET);
> +
> /* Verify that we received this over a legitimate pfkeyv2 socket */
> bzero(headers, sizeof(headers));
>
> @@ -940,8 +942,6 @@ pfkeyv2_send(struct socket *socket, void
> }
> #endif /* IPSEC */
>
> - s = splsoftnet();
> -
> /* Find TDB */
> sa2 = gettdb(rdomain, ssa->sadb_sa_spi, sunionp,
> SADB_X_GETSPROTO(smsg->sadb_msg_satype));
> @@ -949,7 +949,7 @@ pfkeyv2_send(struct socket *socket, void
> /* If there's no such SA, we're done */
> if (sa2 == NULL) {
> rval = ESRCH;
> - goto splxret;
> + goto ret;
> }
>
> /* If this is a reserved SA */
> @@ -969,7 +969,7 @@ pfkeyv2_send(struct socket *socket, void
> &newsa->tdb_sproto, &alg))) {
> tdb_free(freeme);
> freeme = NULL;
> - goto splxret;
> + goto ret;
> }
>
> /* Initialize SA */
> @@ -1020,7 +1020,7 @@ pfkeyv2_send(struct socket *socket, void
> rval = EINVAL;
> tdb_free(freeme);
> freeme = NULL;
> - goto splxret;
> + goto ret;
> }
>
> newsa->tdb_cur_allocations = sa2->tdb_cur_allocations;
> @@ -1041,7 +1041,7 @@ pfkeyv2_send(struct socket *socket, void
> headers[SADB_EXT_IDENTITY_DST] ||
> headers[SADB_EXT_SENSITIVITY]) {
> rval = EINVAL;
> - goto splxret;
> + goto ret;
> }
>
> import_sa(sa2, headers[SADB_EXT_SA], NULL);
> @@ -1059,7 +1059,6 @@ pfkeyv2_send(struct socket *socket, void
> #endif
> }
>
> - splx(s);
> break;
> case SADB_ADD:
> ssa = (struct sadb_sa *) headers[SADB_EXT_SA];
> @@ -1092,21 +1091,19 @@ pfkeyv2_send(struct socket *socket, void
> }
> #endif /* IPSEC */
>
> - s = splsoftnet();
> -
> sa2 = gettdb(rdomain, ssa->sadb_sa_spi, sunionp,
> SADB_X_GETSPROTO(smsg->sadb_msg_satype));
>
> /* We can't add an existing SA! */
> if (sa2 != NULL) {
> rval = EEXIST;
> - goto splxret;
> + goto ret;
> }
>
> /* We can only add "mature" SAs */
> if (ssa->sadb_sa_state != SADB_SASTATE_MATURE) {
> rval = EINVAL;
> -
