> On 22 Oct 2021, at 18:23, Alexander Bluhm <[email protected]> wrote:
>
> On Thu, Oct 21, 2021 at 03:04:22PM +0300, Vitaliy Makkoveev wrote:
>> With this diff the `tdb' dereference after tdb_free() is safe enough and
>> I have no objections against your diff.
>
> My diff did not make things worse, it counted the drops earlier.
> But I decided to drop that part, I would like just do commit the
> error cleanup.
>
> Fixing the tdb destroy is another story. I do not see how the
> NET_LOCK(); NET_UNLOCK() is a fix, it looks more like a hack.
>
Yes, it’s a hack, but it works. We have the only thread which performs
tdb_destroy(). This thread holds exclusive netlock, so adding this
NET_LOCK(); NET_UNLOCK(); sequence to tdb_reaper() enforces it to wait
until the tdb_destroy() thread releases netlock. When tdb_destroy()
thread released netlock this “destroyed” `tdb’ will be not accessible
and it’s safe to release memory.
I like to have this hack committed until the tdb_destroy() fixed.
> ok to commit this error cleanup?
>
> bluhm
>
Yes, I just pointed the fragile place. But since it exists for a long
time and still not triggered I have no objections to commit your diff.
ok mvs@
> Index: netinet/ip_ah.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v
> retrieving revision 1.157
> diff -u -p -r1.157 ip_ah.c
> --- netinet/ip_ah.c 21 Oct 2021 22:59:07 -0000 1.157
> +++ netinet/ip_ah.c 22 Oct 2021 14:41:53 -0000
> @@ -1164,6 +1164,7 @@ ah_output_cb(struct tdb *tdb, struct tdb
> {
> int skip = tc->tc_skip;
> caddr_t ptr = (caddr_t) (tc + 1);
> + int error;
>
> /*
> * Copy original headers (with the new protocol number) back
> @@ -1175,10 +1176,8 @@ ah_output_cb(struct tdb *tdb, struct tdb
> free(tc, M_XDATA, 0);
>
> /* Call the IPsec input callback. */
> - if (ipsp_process_done(m, tdb)) {
> + error = ipsp_process_done(m, tdb);
> + if (error)
> ahstat_inc(ahs_outfail);
> - return -1;
> - }
> -
> - return 0;
> + return error;
> }
> Index: netinet/ip_esp.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_esp.c,v
> retrieving revision 1.176
> diff -u -p -r1.176 ip_esp.c
> --- netinet/ip_esp.c 21 Oct 2021 22:59:07 -0000 1.176
> +++ netinet/ip_esp.c 22 Oct 2021 14:42:27 -0000
> @@ -1024,16 +1024,16 @@ int
> esp_output_cb(struct tdb *tdb, struct tdb_crypto *tc, struct mbuf *m, int
> ilen,
> int olen)
> {
> + int error;
> +
> /* Release crypto descriptors. */
> free(tc, M_XDATA, 0);
>
> /* Call the IPsec input callback. */
> - if (ipsp_process_done(m, tdb)) {
> + error = ipsp_process_done(m, tdb);
> + if (error)
> espstat_inc(esps_outfail);
> - return -1;
> - }
> -
> - return 0;
> + return error;
> }
>
> #define SEEN_SIZE howmany(TDB_REPLAYMAX, 32)
> Index: netinet/ip_ipcomp.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipcomp.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 ip_ipcomp.c
> --- netinet/ip_ipcomp.c 22 Oct 2021 12:30:53 -0000 1.78
> +++ netinet/ip_ipcomp.c 22 Oct 2021 14:42:41 -0000
> @@ -505,6 +505,7 @@ ipcomp_output_cb(struct tdb *tdb, struct
> #ifdef ENCDEBUG
> char buf[INET6_ADDRSTRLEN];
> #endif
> + int error;
>
> skip = tc->tc_skip;
> rlen = ilen - skip;
> @@ -523,7 +524,8 @@ ipcomp_output_cb(struct tdb *tdb, struct
> ipsp_address(&tdb->tdb_dst, buf, sizeof(buf)),
> ntohl(tdb->tdb_spi));
> ipcompstat_inc(ipcomps_wrap);
> - goto baddone;
> + error = ENOBUFS;
> + goto drop;
> }
>
> /* Initialize the IPCOMP header */
> @@ -552,21 +554,21 @@ ipcomp_output_cb(struct tdb *tdb, struct
> ipsp_address(&tdb->tdb_dst, buf, sizeof(buf)),
> ntohl(tdb->tdb_spi));
> ipcompstat_inc(ipcomps_nopf);
> - goto baddone;
> + error = EPFNOSUPPORT;
> + goto drop;
> }
>
> skiphdr:
> /* Release the crypto descriptor. */
> free(tc, M_XDATA, 0);
>
> - if (ipsp_process_done(m, tdb)) {
> + error = ipsp_process_done(m, tdb);
> + if (error)
> ipcompstat_inc(ipcomps_outfail);
> - return -1;
> - }
> - return 0;
> + return error;
>
> - baddone:
> + drop:
> m_freem(m);
> free(tc, M_XDATA, 0);
> - return -1;
> + return error;
> }
> Index: netinet/ip_ipip.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipip.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 ip_ipip.c
> --- netinet/ip_ipip.c 13 Oct 2021 14:36:31 -0000 1.95
> +++ netinet/ip_ipip.c 22 Oct 2021 14:45:33 -0000
> @@ -524,7 +524,7 @@ ipip_output(struct mbuf **mp, struct tdb
> DPRINTF("unsupported protocol family %d",
> tdb->tdb_dst.sa.sa_family);
> ipipstat_inc(ipips_family);
> - error = EAFNOSUPPORT;
> + error = EPFNOSUPPORT;
> goto drop;
> }
>
> Index: netinet/ipsec_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_input.c,v
> retrieving revision 1.184
> diff -u -p -r1.184 ipsec_input.c
> --- netinet/ipsec_input.c 13 Oct 2021 22:49:11 -0000 1.184
> +++ netinet/ipsec_input.c 22 Oct 2021 14:58:54 -0000
> @@ -359,10 +359,10 @@ ipsec_common_input(struct mbuf *m, int s
> return error;
>
> drop:
> + m_freem(m);
> ipsecstat_inc(ipsec_idrops);
> if (tdbp != NULL)
> tdbp->tdb_idrops++;
> - m_freem(m);
> return error;
> }
>
> @@ -423,7 +423,6 @@ ipsec_input_cb(struct cryptop *crp)
> panic("%s: unknown/unsupported security protocol %d",
> __func__, tdb->tdb_sproto);
> }
> -
> if (error) {
> ipsecstat_inc(ipsec_idrops);
> tdb->tdb_idrops++;
> @@ -431,12 +430,12 @@ ipsec_input_cb(struct cryptop *crp)
> return;
>
> drop:
> + m_freem(m);
> + free(tc, M_XDATA, 0);
> + crypto_freereq(crp);
> ipsecstat_inc(ipsec_idrops);
> if (tdb != NULL)
> tdb->tdb_idrops++;
> - free(tc, M_XDATA, 0);
> - m_freem(m);
> - crypto_freereq(crp);
> }
>
> /*
> @@ -448,19 +447,15 @@ ipsec_common_input_cb(struct mbuf *m, st
> {
> int af, sproto;
> u_int8_t prot;
> -
> #if NBPFILTER > 0
> struct ifnet *encif;
> #endif
> -
> struct ip *ip, ipn;
> -
> #ifdef INET6
> struct ip6_hdr *ip6, ip6n;
> #endif /* INET6 */
> struct m_tag *mtag;
> struct tdb_ident *tdbi;
> -
> #ifdef ENCDEBUG
> char buf[INET6_ADDRSTRLEN];
> #endif
> @@ -477,7 +472,7 @@ ipsec_common_input_cb(struct mbuf *m, st
> ipsp_address(&tdbp->tdb_dst, buf, sizeof(buf)),
> ntohl(tdbp->tdb_spi));
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops, ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
>
> ip = mtod(m, struct ip *);
> @@ -489,10 +484,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> /* IP-in-IP encapsulation */
> if (prot == IPPROTO_IPIP) {
> if (m->m_pkthdr.len - skip < sizeof(struct ip)) {
> - m_freem(m);
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
> /* ipn will now contain the inner IPv4 header */
> m_copydata(m, skip, sizeof(struct ip),
> @@ -503,10 +497,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> /* IPv6-in-IP encapsulation. */
> if (prot == IPPROTO_IPV6) {
> if (m->m_pkthdr.len - skip < sizeof(struct ip6_hdr)) {
> - m_freem(m);
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
> /* ip6n will now contain the inner IPv6 header. */
> m_copydata(m, skip, sizeof(struct ip6_hdr),
> @@ -517,8 +510,7 @@ ipsec_common_input_cb(struct mbuf *m, st
>
> #ifdef INET6
> /* Fix IPv6 header */
> - if (af == AF_INET6)
> - {
> + if (af == AF_INET6) {
> if (m->m_len < sizeof(struct ip6_hdr) &&
> (m = m_pullup(m, sizeof(struct ip6_hdr))) == NULL) {
>
> @@ -526,7 +518,7 @@ ipsec_common_input_cb(struct mbuf *m, st
> ipsp_address(&tdbp->tdb_dst, buf, sizeof(buf)),
> ntohl(tdbp->tdb_spi));
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops, ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
>
> ip6 = mtod(m, struct ip6_hdr *);
> @@ -538,10 +530,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> /* IP-in-IP encapsulation */
> if (prot == IPPROTO_IPIP) {
> if (m->m_pkthdr.len - skip < sizeof(struct ip)) {
> - m_freem(m);
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
> /* ipn will now contain the inner IPv4 header */
> m_copydata(m, skip, sizeof(struct ip), (caddr_t) &ipn);
> @@ -550,10 +541,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> /* IPv6-in-IP encapsulation */
> if (prot == IPPROTO_IPV6) {
> if (m->m_pkthdr.len - skip < sizeof(struct ip6_hdr)) {
> - m_freem(m);
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
> /* ip6n will now contain the inner IPv6 header. */
> m_copydata(m, skip, sizeof(struct ip6_hdr),
> @@ -574,10 +564,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> switch (prot) {
> case IPPROTO_UDP:
> if (m->m_pkthdr.len < skip + sizeof(struct udphdr)) {
> - m_freem(m);
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
> cksum = 0;
> m_copyback(m, skip + offsetof(struct udphdr, uh_sum),
> @@ -593,10 +582,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> break;
> case IPPROTO_TCP:
> if (m->m_pkthdr.len < skip + sizeof(struct tcphdr)) {
> - m_freem(m);
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
> cksum = 0;
> m_copyback(m, skip + offsetof(struct tcphdr, th_sum),
> @@ -623,10 +611,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> mtag = m_tag_get(PACKET_TAG_IPSEC_IN_DONE,
> sizeof(struct tdb_ident), M_NOWAIT);
> if (mtag == NULL) {
> - m_freem(m);
> DPRINTF("failed to get tag");
> IPSEC_ISTAT(esps_hdrops, ahs_hdrops, ipcomps_hdrops);
> - return -1;
> + goto baddone;
> }
>
> tdbi = (struct tdb_ident *)(mtag + 1);
> @@ -703,22 +690,24 @@ ipsec_common_input_cb(struct mbuf *m, st
>
> /* This is the enc0 interface unless for ipcomp. */
> if ((ifp = if_get(m->m_pkthdr.ph_ifidx)) == NULL) {
> - m_freem(m);
> - return -1;
> + goto baddone;
> }
> if (pf_test(af, PF_IN, ifp, &m) != PF_PASS) {
> if_put(ifp);
> - m_freem(m);
> - return -1;
> + goto baddone;
> }
> if_put(ifp);
> if (m == NULL)
> - return -1;
> + return 0;
> }
> #endif
> /* Call the appropriate IPsec transform callback. */
> ip_deliver(&m, &skip, prot, af);
> return 0;
> +
> + baddone:
> + m_freem(m);
> + return -1;
> #undef IPSEC_ISTAT
> }
>
> Index: netinet/ipsec_output.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_output.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 ipsec_output.c
> --- netinet/ipsec_output.c 13 Oct 2021 22:43:44 -0000 1.89
> +++ netinet/ipsec_output.c 22 Oct 2021 15:02:38 -0000
> @@ -130,7 +130,7 @@ ipsp_process_packet(struct mbuf *m, stru
> ipsp_address(&tdb->tdb_dst, buf, sizeof(buf)),
> ntohl(tdb->tdb_spi), tdb->tdb_sproto,
> tdb->tdb_dst.sa.sa_family);
> - error = ENXIO;
> + error = EPFNOSUPPORT;
> goto drop;
> }
>
> @@ -348,7 +348,7 @@ ipsp_process_packet(struct mbuf *m, stru
> break;
> #endif /* INET6 */
> default:
> - error = EINVAL;
> + error = EPFNOSUPPORT;
> goto drop;
> }
>
> @@ -434,10 +434,9 @@ ipsec_output_cb(struct cryptop *crp)
> error = ipcomp_output_cb(tdb, tc, m, ilen, olen);
> break;
> default:
> - panic("%s: unknown/unsupported security protocol %d",
> + panic("%s: unhandled security protocol %d",
> __func__, tdb->tdb_sproto);
> }
> -
> if (error) {
> ipsecstat_inc(ipsec_odrops);
> tdb->tdb_odrops++;
> @@ -445,12 +444,12 @@ ipsec_output_cb(struct cryptop *crp)
> return;
>
> drop:
> - if (tdb != NULL)
> - tdb->tdb_odrops++;
> m_freem(m);
> free(tc, M_XDATA, 0);
> crypto_freereq(crp);
> ipsecstat_inc(ipsec_odrops);
> + if (tdb != NULL)
> + tdb->tdb_odrops++;
> }
>
> /*
> @@ -494,7 +493,7 @@ ipsp_process_done(struct mbuf *m, struct
> default:
> DPRINTF("unknown protocol family (%d)",
> tdb->tdb_dst.sa.sa_family);
> - error = ENXIO;
> + error = EPFNOSUPPORT;
> goto drop;
> }
>
> @@ -548,7 +547,7 @@ ipsp_process_done(struct mbuf *m, struct
> default:
> DPRINTF("unknown protocol family (%d)",
> tdb->tdb_dst.sa.sa_family);
> - error = ENXIO;
> + error = EPFNOSUPPORT;
> goto drop;
> }
>
> @@ -596,18 +595,22 @@ ipsp_process_done(struct mbuf *m, struct
> */
> switch (tdb->tdb_dst.sa.sa_family) {
> case AF_INET:
> - return (ip_output(m, NULL, NULL, IP_RAWOUTPUT, NULL, NULL, 0));
> -
> + error = ip_output(m, NULL, NULL, IP_RAWOUTPUT, NULL, NULL, 0);
> + break;
> #ifdef INET6
> case AF_INET6:
> /*
> * We don't need massage, IPv6 header fields are always in
> * net endian.
> */
> - return (ip6_output(m, NULL, NULL, 0, NULL, NULL));
> + error = ip6_output(m, NULL, NULL, 0, NULL, NULL);
> + break;
> #endif /* INET6 */
> + default:
> + error = EPFNOSUPPORT;
> + break;
> }
> - error = EINVAL; /* Not reached. */
> + return error;
>
> drop:
> m_freem(m);
