On Mon, Apr 24, 2023 at 06:31:03PM +0300, Vitaliy Makkoveev wrote:
> ... and use it to protect route labels storage. This time it is not
> clean, which lock protects it because we holding kernel and net locks in
> various combinations while accessing it. I see no reason to put kernel
> lock to all the places. Also netlock could not be used, because rtfree()
> 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, I want to
> 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?
I did run this though perform test. Full regress still running due
to unrelated tree breakage. I don't expect fallout.
OK bluhm@
> Index: sys/net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.692
> diff -u -p -r1.692 if.c
> --- sys/net/if.c 22 Apr 2023 04:39:46 -0000 1.692
> +++ sys/net/if.c 24 Apr 2023 15:11:33 -0000
> @@ -2383,7 +2383,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:
> @@ -2458,9 +2457,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
> Index: sys/net/pf_ioctl.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.397
> diff -u -p -r1.397 pf_ioctl.c
> --- sys/net/pf_ioctl.c 6 Jan 2023 17:44:34 -0000 1.397
> +++ sys/net/pf_ioctl.c 24 Apr 2023 15:11:33 -0000
> @@ -491,14 +491,10 @@ 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));
> }
> }
> Index: sys/net/route.c
> ===================================================================
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.416
> diff -u -p -r1.416 route.c
> --- sys/net/route.c 28 Jan 2023 10:17:16 -0000 1.416
> +++ sys/net/route.c 24 Apr 2023 15:11:33 -0000
> @@ -113,6 +113,7 @@
> #include <sys/queue.h>
> #include <sys/pool.h>
> #include <sys/atomic.h>
> +#include <sys/mutex.h>
>
> #include <net/if.h>
> #include <net/if_var.h>
> @@ -137,6 +138,12 @@
> #include <net/bfd.h>
> #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 *,
> #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)
> @@ -1603,15 +1612,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;
> }
>
> /*
> @@ -1625,11 +1636,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++;
> @@ -1639,14 +1650,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);
> @@ -1654,18 +1671,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);
> }
> @@ -1678,6 +1721,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) {
> @@ -1687,6 +1731,7 @@ rtlabel_unref(u_int16_t id)
> break;
> }
> }
> + mtx_leave(&rtlabel_mtx);
> }
>
> int
> Index: sys/net/route.h
> ===================================================================
> RCS file: /cvs/src/sys/net/route.h,v
> retrieving revision 1.198
> diff -u -p -r1.198 route.h
> --- sys/net/route.h 28 Jan 2023 10:17:16 -0000 1.198
> +++ sys/net/route.h 24 Apr 2023 15:11:33 -0000
> @@ -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);
