From b46ef14d0f90f345b5e0d02eb410767017a09f42 Mon Sep 17 00:00:00 2001 From: mvs Date: Wed, 26 Apr 2023 19:54:35 +0000 Subject: [PATCH] Introduce `rtlabel_mtx' mutex(9) to protect route labels storage. This time kernel and net locks are held in various combination to protect it. We don't want to put kernel lock to all the places. Netlock also can't be used because rtfree(9) which calls rtlabel_unref() has unknown netlock state within. This new `rtlabel_mtx' mutex(9) protects `rt_labels' list and `label' entry dereference. Since we don't export 'rt_label' structure, keep this lock private to net/route.c. For this reason rtlabel_id2name() now copies label string to externally passed buffer instead of returning address of `rt_labels' list data. This is the way which rtlabel_id2sa() already works. ok bluhm@ --- sys/net/if.c | 8 ++--- sys/net/pf_ioctl.c | 10 ++---- sys/net/route.c | 79 ++++++++++++++++++++++++++++++++++++---------- sys/net/route.h | 5 +-- 4 files changed, 71 insertions(+), 31 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index 58db412810d..4534ed54faa 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if.c,v 1.693 2023/04/26 00:14:21 jan Exp $ */ +/* $OpenBSD: if.c,v 1.694 2023/04/26 19:54:35 mvs Exp $ */ /* $NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $ */ /* @@ -2388,7 +2388,6 @@ ifioctl_get(u_long cmd, caddr_t data) char ifrtlabelbuf[RTLABEL_LEN]; int error = 0; size_t bytesdone; - const char *label; switch(cmd) { case SIOCGIFCONF: @@ -2463,9 +2462,8 @@ ifioctl_get(u_long cmd, caddr_t data) break; case SIOCGIFRTLABEL: - if (ifp->if_rtlabelid && - (label = rtlabel_id2name(ifp->if_rtlabelid)) != NULL) { - strlcpy(ifrtlabelbuf, label, RTLABEL_LEN); + if (ifp->if_rtlabelid && rtlabel_id2name(ifp->if_rtlabelid, + ifrtlabelbuf, RTLABEL_LEN) != NULL) { error = copyoutstr(ifrtlabelbuf, ifr->ifr_data, RTLABEL_LEN, &bytesdone); } else diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c index 1141069dcf6..8045a818d03 100644 --- a/sys/net/pf_ioctl.c +++ b/sys/net/pf_ioctl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_ioctl.c,v 1.397 2023/01/06 17:44:34 sashan Exp $ */ +/* $OpenBSD: pf_ioctl.c,v 1.398 2023/04/26 19:54:35 mvs Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -491,15 +491,11 @@ pf_rtlabel_remove(struct pf_addr_wrap *a) void pf_rtlabel_copyout(struct pf_addr_wrap *a) { - const char *name; - if (a->type == PF_ADDR_RTLABEL && a->v.rtlabel) { - if ((name = rtlabel_id2name(a->v.rtlabel)) == NULL) + if (rtlabel_id2name(a->v.rtlabel, a->v.rtlabelname, + sizeof(a->v.rtlabelname)) == NULL) strlcpy(a->v.rtlabelname, "?", sizeof(a->v.rtlabelname)); - else - strlcpy(a->v.rtlabelname, name, - sizeof(a->v.rtlabelname)); } } diff --git a/sys/net/route.c b/sys/net/route.c index a3de25d7b24..485928ef618 100644 --- a/sys/net/route.c +++ b/sys/net/route.c @@ -1,4 +1,4 @@ -/* $OpenBSD: route.c,v 1.418 2023/04/26 16:09:44 kn Exp $ */ +/* $OpenBSD: route.c,v 1.419 2023/04/26 19:54:35 mvs Exp $ */ /* $NetBSD: route.c,v 1.14 1996/02/13 22:00:46 christos Exp $ */ /* @@ -113,6 +113,7 @@ #include #include #include +#include #include #include @@ -137,6 +138,12 @@ #include #endif +/* + * Locks used to protect struct members: + * I immutable after creation + * L rtlabel_mtx + */ + #define ROUNDUP(a) (a>0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long)) /* Give some jitter to hash, to avoid synchronization between routers. */ @@ -163,13 +170,15 @@ static int rt_copysa(struct sockaddr *, struct sockaddr *, struct sockaddr **); #define LABELID_MAX 50000 struct rt_label { - TAILQ_ENTRY(rt_label) rtl_entry; - char rtl_name[RTLABEL_LEN]; - u_int16_t rtl_id; - int rtl_ref; + TAILQ_ENTRY(rt_label) rtl_entry; /* [L] */ + char rtl_name[RTLABEL_LEN]; /* [I] */ + u_int16_t rtl_id; /* [I] */ + int rtl_ref; /* [L] */ }; -TAILQ_HEAD(rt_labels, rt_label) rt_labels = TAILQ_HEAD_INITIALIZER(rt_labels); +TAILQ_HEAD(rt_labels, rt_label) rt_labels = + TAILQ_HEAD_INITIALIZER(rt_labels); /* [L] */ +struct mutex rtlabel_mtx = MUTEX_INITIALIZER(IPL_NET); void route_init(void) @@ -1596,15 +1605,17 @@ u_int16_t rtlabel_name2id(char *name) { struct rt_label *label, *p; - u_int16_t new_id = 1; + u_int16_t new_id = 1, id = 0; if (!name[0]) return (0); + mtx_enter(&rtlabel_mtx); TAILQ_FOREACH(label, &rt_labels, rtl_entry) if (strcmp(name, label->rtl_name) == 0) { label->rtl_ref++; - return (label->rtl_id); + id = label->rtl_id; + goto out; } /* @@ -1618,11 +1629,11 @@ rtlabel_name2id(char *name) new_id = p->rtl_id + 1; } if (new_id > LABELID_MAX) - return (0); + goto out; label = malloc(sizeof(*label), M_RTABLE, M_NOWAIT|M_ZERO); if (label == NULL) - return (0); + goto out; strlcpy(label->rtl_name, name, sizeof(label->rtl_name)); label->rtl_id = new_id; label->rtl_ref++; @@ -1632,14 +1643,20 @@ rtlabel_name2id(char *name) else /* either list empty or no free slot in between */ TAILQ_INSERT_TAIL(&rt_labels, label, rtl_entry); - return (label->rtl_id); + id = label->rtl_id; +out: + mtx_leave(&rtlabel_mtx); + + return (id); } const char * -rtlabel_id2name(u_int16_t id) +rtlabel_id2name_locked(u_int16_t id) { struct rt_label *label; + MUTEX_ASSERT_LOCKED(&rtlabel_mtx); + TAILQ_FOREACH(label, &rt_labels, rtl_entry) if (label->rtl_id == id) return (label->rtl_name); @@ -1647,18 +1664,44 @@ rtlabel_id2name(u_int16_t id) return (NULL); } +const char * +rtlabel_id2name(u_int16_t id, char *rtlabelbuf, size_t sz) +{ + const char *label; + + if (id == 0) + return (NULL); + + mtx_enter(&rtlabel_mtx); + if ((label = rtlabel_id2name_locked(id)) != NULL) + strlcpy(rtlabelbuf, label, sz); + mtx_leave(&rtlabel_mtx); + + if (label == NULL) + return (NULL); + + return (rtlabelbuf); +} + struct sockaddr * rtlabel_id2sa(u_int16_t labelid, struct sockaddr_rtlabel *sa_rl) { const char *label; - if (labelid == 0 || (label = rtlabel_id2name(labelid)) == NULL) + if (labelid == 0) return (NULL); - bzero(sa_rl, sizeof(*sa_rl)); - sa_rl->sr_len = sizeof(*sa_rl); - sa_rl->sr_family = AF_UNSPEC; - strlcpy(sa_rl->sr_label, label, sizeof(sa_rl->sr_label)); + mtx_enter(&rtlabel_mtx); + if ((label = rtlabel_id2name_locked(labelid)) != NULL) { + bzero(sa_rl, sizeof(*sa_rl)); + sa_rl->sr_len = sizeof(*sa_rl); + sa_rl->sr_family = AF_UNSPEC; + strlcpy(sa_rl->sr_label, label, sizeof(sa_rl->sr_label)); + } + mtx_leave(&rtlabel_mtx); + + if (label == NULL) + return (NULL); return ((struct sockaddr *)sa_rl); } @@ -1671,6 +1714,7 @@ rtlabel_unref(u_int16_t id) if (id == 0) return; + mtx_enter(&rtlabel_mtx); TAILQ_FOREACH_SAFE(p, &rt_labels, rtl_entry, next) { if (id == p->rtl_id) { if (--p->rtl_ref == 0) { @@ -1680,6 +1724,7 @@ rtlabel_unref(u_int16_t id) break; } } + mtx_leave(&rtlabel_mtx); } int diff --git a/sys/net/route.h b/sys/net/route.h index 1476ac35956..e74d623fc0b 100644 --- a/sys/net/route.h +++ b/sys/net/route.h @@ -1,4 +1,4 @@ -/* $OpenBSD: route.h,v 1.198 2023/01/28 10:17:16 mvs Exp $ */ +/* $OpenBSD: route.h,v 1.199 2023/04/26 19:54:35 mvs Exp $ */ /* $NetBSD: route.h,v 1.9 1996/02/13 22:00:49 christos Exp $ */ /* @@ -416,7 +416,8 @@ struct rttimer_queue { int rtq_timeout; /* [T] */ }; -const char *rtlabel_id2name(u_int16_t); +const char *rtlabel_id2name_locked(u_int16_t); +const char *rtlabel_id2name(u_int16_t, char *, size_t); u_int16_t rtlabel_name2id(char *); struct sockaddr *rtlabel_id2sa(u_int16_t, struct sockaddr_rtlabel *); void rtlabel_unref(u_int16_t); -- 2.20.1