On Thu, Jul 14, 2022 at 01:46:14PM +0200, Alexander Bluhm wrote:
> Hi,
>
> Protect all writers to ifm_cur with a mutex. ifmedia_match() does
> not return any pointers without lock anymore.
>
> ok?
I like to use fmedia_get() instead fmedia_match_get(). With this change
ok mvs@.
>
> bluhm
>
> Index: dev/ic/if_wi.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/dev/ic/if_wi.c,v
> retrieving revision 1.176
> diff -u -p -r1.176 if_wi.c
> --- dev/ic/if_wi.c 9 Jan 2022 05:42:38 -0000 1.176
> +++ dev/ic/if_wi.c 13 Jul 2022 14:15:18 -0000
> @@ -2707,7 +2707,7 @@ wi_sync_media(struct wi_softc *sc, int p
> }
> media = IFM_MAKEWORD(IFM_TYPE(media), subtype, options,
> IFM_INST(media));
> - if (ifmedia_match(&sc->sc_media, media, sc->sc_media.ifm_mask) == NULL)
> + if (!ifmedia_match(&sc->sc_media, media, sc->sc_media.ifm_mask))
> return (EINVAL);
> ifmedia_set(&sc->sc_media, media);
> sc->wi_ptype = ptype;
> Index: net/if_media.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_media.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 if_media.c
> --- net/if_media.c 12 Jul 2022 22:27:38 -0000 1.35
> +++ net/if_media.c 13 Jul 2022 14:19:29 -0000
> @@ -105,6 +105,8 @@ static void ifmedia_printword(uint64_t);
>
> struct mutex ifmedia_mtx = MUTEX_INITIALIZER(IPL_NET);
>
> +struct ifmedia_entry *ifmedia_match_get(struct ifmedia *, uint64_t,
> uint64_t);
> +
> /*
> * Initialize if_media struct for a specific interface instance.
> */
> @@ -181,7 +183,8 @@ ifmedia_set(struct ifmedia *ifm, uint64_
> {
> struct ifmedia_entry *match;
>
> - match = ifmedia_match(ifm, target, ifm->ifm_mask);
> + mtx_enter(&ifmedia_mtx);
> + match = ifmedia_match_get(ifm, target, ifm->ifm_mask);
>
> /*
> * If we didn't find the requested media, then we try to fall
> @@ -201,15 +204,20 @@ ifmedia_set(struct ifmedia *ifm, uint64_
> printf("%s: no match for 0x%llx/0x%llx\n", __func__,
> target, ~ifm->ifm_mask);
> target = (target & IFM_NMASK) | IFM_NONE;
> - match = ifmedia_match(ifm, target, ifm->ifm_mask);
> + match = ifmedia_match_get(ifm, target, ifm->ifm_mask);
> if (match == NULL) {
> + mtx_leave(&ifmedia_mtx);
> ifmedia_add(ifm, target, 0, NULL);
> - match = ifmedia_match(ifm, target, ifm->ifm_mask);
> - if (match == NULL)
> + mtx_enter(&ifmedia_mtx);
> + match = ifmedia_match_get(ifm, target, ifm->ifm_mask);
> + if (match == NULL) {
> + mtx_leave(&ifmedia_mtx);
> panic("ifmedia_set failed");
> + }
> }
> }
> ifm->ifm_cur = match;
> + mtx_leave(&ifmedia_mtx);
>
> #ifdef IFMEDIA_DEBUG
> if (ifmedia_debug) {
> @@ -245,8 +253,10 @@ ifmedia_ioctl(struct ifnet *ifp, struct
> uint64_t oldmedia;
> uint64_t newmedia = ifr->ifr_media;
>
> - match = ifmedia_match(ifm, newmedia, ifm->ifm_mask);
> + mtx_enter(&ifmedia_mtx);
> + match = ifmedia_match_get(ifm, newmedia, ifm->ifm_mask);
> if (match == NULL) {
> + mtx_leave(&ifmedia_mtx);
> #ifdef IFMEDIA_DEBUG
> if (ifmedia_debug) {
> printf("%s: no media found for 0x%llx\n",
> @@ -264,8 +274,10 @@ ifmedia_ioctl(struct ifnet *ifp, struct
> */
> if ((IFM_SUBTYPE(newmedia) != IFM_AUTO) &&
> (newmedia == ifm->ifm_media) &&
> - (match == ifm->ifm_cur))
> + (match == ifm->ifm_cur)) {
> + mtx_leave(&ifmedia_mtx);
> return (0);
> + }
>
> /*
> * We found a match, now make the driver switch to it.
> @@ -283,10 +295,16 @@ ifmedia_ioctl(struct ifnet *ifp, struct
> oldmedia = ifm->ifm_media;
> ifm->ifm_cur = match;
> ifm->ifm_media = newmedia;
> + mtx_leave(&ifmedia_mtx);
> +
> error = (*ifm->ifm_change_cb)(ifp);
> if (error && error != ENETRESET) {
> - ifm->ifm_cur = oldentry;
> - ifm->ifm_media = oldmedia;
> + mtx_enter(&ifmedia_mtx);
> + if (ifm->ifm_cur == match) {
> + ifm->ifm_cur = oldentry;
> + ifm->ifm_media = oldmedia;
> + }
> + mtx_leave(&ifmedia_mtx);
> }
> break;
> }
> @@ -302,10 +320,13 @@ ifmedia_ioctl(struct ifnet *ifp, struct
> if (ifmr->ifm_count < 0)
> return (EINVAL);
>
> + mtx_enter(&ifmedia_mtx);
> ifmr->ifm_active = ifmr->ifm_current = ifm->ifm_cur ?
> ifm->ifm_cur->ifm_media : IFM_NONE;
> ifmr->ifm_mask = ifm->ifm_mask;
> ifmr->ifm_status = 0;
> + mtx_leave(&ifmedia_mtx);
> +
> (*ifm->ifm_status_cb)(ifp, ifmr);
>
> mtx_enter(&ifmedia_mtx);
> @@ -368,17 +389,30 @@ ifmedia_ioctl(struct ifnet *ifp, struct
> }
>
> /*
> - * Find media entry matching a given ifm word.
> + * Find media entry matching a given ifm word. Return 1 if found.
> */
> -struct ifmedia_entry *
> +int
> ifmedia_match(struct ifmedia *ifm, uint64_t target, uint64_t mask)
> {
> + struct ifmedia_entry *match;
> +
> + mtx_enter(&ifmedia_mtx);
> + match = ifmedia_match_get(ifm, target, mask);
> + mtx_leave(&ifmedia_mtx);
> +
> + return (match != NULL);
> +}
> +
> +struct ifmedia_entry *
> +ifmedia_match_get(struct ifmedia *ifm, uint64_t target, uint64_t mask)
> +{
> struct ifmedia_entry *match, *next;
>
> + MUTEX_ASSERT_LOCKED(&ifmedia_mtx);
> +
> match = NULL;
> mask = ~mask;
>
> - mtx_enter(&ifmedia_mtx);
> TAILQ_FOREACH(next, &ifm->ifm_list, ifm_list) {
> if ((next->ifm_media & mask) == (target & mask)) {
> if (match) {
> @@ -392,7 +426,6 @@ ifmedia_match(struct ifmedia *ifm, uint6
> match = next;
> }
> }
> - mtx_leave(&ifmedia_mtx);
>
> return (match);
> }
> @@ -417,6 +450,7 @@ ifmedia_delete_instance(struct ifmedia *
> TAILQ_INSERT_TAIL(&ifmlist, ife, ifm_list);
> }
> }
> + ifm->ifm_cur = NULL;
> mtx_leave(&ifmedia_mtx);
>
> /* Do not hold mutex longer than necessary, call free() without. */
> Index: net/if_media.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_media.h,v
> retrieving revision 1.44
> diff -u -p -r1.44 if_media.h
> --- net/if_media.h 12 Jul 2022 22:08:18 -0000 1.44
> +++ net/if_media.h 13 Jul 2022 14:18:09 -0000
> @@ -103,7 +103,7 @@ TAILQ_HEAD(ifmedia_list, ifmedia_entry);
> struct ifmedia {
> uint64_t ifm_mask; /* mask of changes we don't care about
> */
> uint64_t ifm_media; /* current user-set media word */
> - struct ifmedia_entry *ifm_cur; /* currently selected media */
> + struct ifmedia_entry *ifm_cur; /* [M] currently selected media */
> struct ifmedia_list ifm_list; /* [M] list of all supported media */
> size_t ifm_nwords; /* [M] number of ifm_list entries */
> ifm_change_cb_t ifm_change_cb; /* media change driver callback */
> @@ -129,7 +129,7 @@ int ifmedia_ioctl(struct ifnet *, struct
> u_long);
>
> /* Locate a media entry */
> -struct ifmedia_entry *ifmedia_match(struct ifmedia *, uint64_t,
> uint64_t);
> +int ifmedia_match(struct ifmedia *, uint64_t, uint64_t);
>
> /* Delete all media for a given media instance */
> void ifmedia_delete_instance(struct ifmedia *, uint64_t);
>
