On Mon, May 14, 2018 at 10:24:09AM +0200, Martin Pieuchot wrote:
> On 14/05/18(Mon) 10:17, Theo Buehler wrote:
> > This was pointed out to me by mpi a while ago.
> >
> > The if_clone_list() function accesses &if_cloners, which is only
> > modified via if_clone_attach() and if_clone_detach().
> >
> > The latter function is never called, as far as I can tell, while
> > if_clone_attach() is only called at boot time via the pdevinit[]
> > mechanism in ioconf.c.
> >
> > So, for the purposes of the NET_LOCK(), &if_cloners is immutable, hence
> > if_clone_list() does not need the NET_RLOCK().
>
> Please add a KERNEL_ASSERT_LOCKED() in if_clone_list(), with that ok
> mpi@.
[...]
> Another (better) alternative would be to introduce a rwlock to protect
> those two globals and use it where required.
Here's an updated diff that implements an rwlock to protect if_cloners
and if_clone_count.
Note that there are two uses for SIOCIFGCLONERS, one that just asks for
a count of cloners and the other that asks for the entire list (compare
with the two ioctl calls in list_cloners() in ifconfig.c). I decided to
split the paths more clearly which also has the benefit of keeping the
diff smaller.
While here, replace a ternary operator with a MIN for readability.
Index: sys/net/if.c
===================================================================
RCS file: /var/cvs/src/sys/net/if.c,v
retrieving revision 1.551
diff -u -p -r1.551 if.c
--- sys/net/if.c 28 Apr 2018 15:44:59 -0000 1.551
+++ sys/net/if.c 14 May 2018 12:32:28 -0000
@@ -217,6 +217,9 @@ void if_idxmap_insert(struct ifnet *);
void if_idxmap_remove(struct ifnet *);
TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head);
+
+/* Serialize access to &if_cloners and if_cloners_count */
+struct rwlock if_cloners_lock = RWLOCK_INITIALIZER("ifclonerslk");
LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners);
int if_cloners_count;
@@ -1243,11 +1246,13 @@ if_clone_lookup(const char *name, int *u
if (cp - name < IFNAMSIZ-1 && *cp == '0' && cp[1] != '\0')
return (NULL); /* unit number 0 padded */
+ rw_enter_read(&if_cloners_lock);
LIST_FOREACH(ifc, &if_cloners, ifc_list) {
if (strlen(ifc->ifc_name) == cp - name &&
!strncmp(name, ifc->ifc_name, cp - name))
break;
}
+ rw_exit_read(&if_cloners_lock);
if (ifc == NULL)
return (NULL);
@@ -1273,8 +1278,10 @@ if_clone_lookup(const char *name, int *u
void
if_clone_attach(struct if_clone *ifc)
{
+ rw_enter_write(&if_cloners_lock);
LIST_INSERT_HEAD(&if_cloners, ifc, ifc_list);
if_cloners_count++;
+ rw_exit_write(&if_cloners_lock);
}
/*
@@ -1283,9 +1290,10 @@ if_clone_attach(struct if_clone *ifc)
void
if_clone_detach(struct if_clone *ifc)
{
-
+ rw_enter_write(&if_cloners_lock);
LIST_REMOVE(ifc, ifc_list);
if_cloners_count--;
+ rw_exit_write(&if_cloners_lock);
}
/*
@@ -1298,17 +1306,21 @@ if_clone_list(struct if_clonereq *ifcr)
struct if_clone *ifc;
int count, error = 0;
- ifcr->ifcr_total = if_cloners_count;
if ((dst = ifcr->ifcr_buffer) == NULL) {
/* Just asking how many there are. */
+ rw_enter_read(&if_cloners_lock);
+ ifcr->ifcr_total = if_cloners_count;
+ rw_exit_read(&if_cloners_lock);
return (0);
}
if (ifcr->ifcr_count < 0)
return (EINVAL);
- count = (if_cloners_count < ifcr->ifcr_count) ?
- if_cloners_count : ifcr->ifcr_count;
+ rw_enter_read(&if_cloners_lock);
+
+ ifcr->ifcr_total = if_cloners_count;
+ count = MIN(if_cloners_count, ifcr->ifcr_count);
LIST_FOREACH(ifc, &if_cloners, ifc_list) {
if (count == 0)
@@ -1322,6 +1334,7 @@ if_clone_list(struct if_clonereq *ifcr)
dst += IFNAMSIZ;
}
+ rw_exit_read(&if_cloners_lock);
return (error);
}
@@ -2164,9 +2177,7 @@ ifioctl_get(u_long cmd, caddr_t data)
NET_RUNLOCK();
return (error);
case SIOCIFGCLONERS:
- NET_RLOCK();
error = if_clone_list((struct if_clonereq *)data);
- NET_RUNLOCK();
return (error);
case SIOCGIFGMEMB:
NET_RLOCK();
