On Thu, Oct 31, 2013 at 09:56:11AM +0100, Martin Pieuchot wrote:
> On 30/10/13(Wed) 16:48, Alexander Bluhm wrote:
> > Now I use the if_index to detect that the interface is gone.
>
> Do you know if the memory pointed by the "imm" pointer you're passing to
> your workq can be freed before the task got scheduled, if the interface
> is removed/destroyed for example?
I am pretty sure that this works. The old in6_leavegroup() code
called free on it, so it must not be referenced anywhere else. All
imm pointer come from the ia6_memberships list, it is emptied while
calling in6_leavegroup().
> There's also a risk of calling in6_leavegroup() a second time before the
> first task get scheduled, in this case the second task will have a bogus
> "imm" pointer. This problem can be avoided by using a task(9) though.
in6_leavegroup() is only called once per imm as it is the in6_multi_mship
free function.
Note that struct in6_multi has a reference counter, so imm->i6mm_maddr
should be valid memory. Also in6_delmulti() itself calls splsoftnet()
so there never was protection against timeout or softnet interrupts
calling in6_leavegroup() again.
> I'm not sure to get the whole picture about this ioctl called in an
> interrupt context so feel free to correct me, but the only offending
> function is nd6_timer(), is that right?
No, this code is also called from softnet interrupt context. An
autoconfigured IPv6 address can deleted by a timeout or by a router
advertisement.
nd6_ra_input() -> prelist_update() -> nd6_prelist_add() ->
purge_detached() -> in6_purgeaddr() -> in6_leavegroup()
In theory we can ignore the incomming packet, we may always loose
packets and hope for a retransmit. But that would mean to avoid
the sleep and do a rollback. That would be a horrible diff.
> Finally since we are moving away from workq(9) to task(9) I'd suggest
> you to use the latter.
That is easy, but it does not solve the problems you describe. I
have to call task_set() in in6_leavegroup() to set the if_index.
May be calling task_set() can be moved to somewhere else but that
does not look easy.
I am still not happy with this diff although I think it does not
cause memory corruption. Look at this call path:
if_detach() -> in6_ifdetach() -> in6_purgeaddr() -> in6_leavegroup()
As I delay the SIOCDELMULTI after the interface is destroyed,
the ioctl is never called.
Perhaps we look at the problem from the wrong direction. Why do
some network drivers sleep when changing multicast groups? Did
they always do that or was something changed?
bluhm
Index: netinet6/in6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.122
diff -u -p -r1.122 in6.c
--- netinet6/in6.c 24 Oct 2013 11:20:18 -0000 1.122
+++ netinet6/in6.c 31 Oct 2013 14:40:05 -0000
@@ -124,6 +124,7 @@ int in6_lifaddr_ioctl(struct socket *, u
int in6_ifinit(struct ifnet *, struct in6_ifaddr *, int);
void in6_unlink_ifa(struct in6_ifaddr *, struct ifnet *);
void in6_ifloop_request(int, struct ifaddr *);
+void in6_leavegroup_task(void *, void *);
const struct sockaddr_in6 sa6_any = {
sizeof(sa6_any), AF_INET6, 0, 0, IN6ADDR_ANY_INIT, 0
@@ -1777,16 +1778,18 @@ in6_delmulti(struct in6_multi *in6m)
ifafree(&in6m->in6m_ia->ia_ifa); /* release reference */
}
- /*
- * Notify the network driver to update its multicast
- * reception filter.
- */
- bzero(&ifr.ifr_addr, sizeof(struct sockaddr_in6));
- ifr.ifr_addr.sin6_len = sizeof(struct sockaddr_in6);
- ifr.ifr_addr.sin6_family = AF_INET6;
- ifr.ifr_addr.sin6_addr = in6m->in6m_addr;
- (*in6m->in6m_ifp->if_ioctl)(in6m->in6m_ifp,
- SIOCDELMULTI, (caddr_t)&ifr);
+ if (in6m->in6m_ifp != NULL) {
+ /*
+ * Notify the network driver to update its multicast
+ * reception filter.
+ */
+ bzero(&ifr.ifr_addr, sizeof(struct sockaddr_in6));
+ ifr.ifr_addr.sin6_len = sizeof(struct sockaddr_in6);
+ ifr.ifr_addr.sin6_family = AF_INET6;
+ ifr.ifr_addr.sin6_addr = in6m->in6m_addr;
+ (*in6m->in6m_ifp->if_ioctl)(in6m->in6m_ifp,
+ SIOCDELMULTI, (caddr_t)&ifr);
+ }
free(in6m, M_IPMADDR);
}
splx(s);
@@ -1814,11 +1817,27 @@ in6_joingroup(struct ifnet *ifp, struct
int
in6_leavegroup(struct in6_multi_mship *imm)
{
+ if (imm->i6mm_maddr) {
+ task_set(&imm->i6mm_task, in6_leavegroup_task, imm,
+ (void *)(unsigned long)imm->i6mm_maddr->in6m_ifp->if_index);
+ task_add(systq, &imm->i6mm_task);
+ } else
+ free(imm, M_IPMADDR);
+ return 0;
+}
+
+void
+in6_leavegroup_task(void *arg1, void *arg2)
+{
+ struct in6_multi_mship *imm = arg1;
+ unsigned int index = (unsigned long)arg2;
- if (imm->i6mm_maddr)
- in6_delmulti(imm->i6mm_maddr);
+ /* If interface has been be freed, avoid call to if_ioctl(). */
+ if (if_get(index) != imm->i6mm_maddr->in6m_ifp)
+ imm->i6mm_maddr->in6m_ifp = NULL;
+
+ in6_delmulti(imm->i6mm_maddr);
free(imm, M_IPMADDR);
- return 0;
}
/*
Index: netinet6/in6_var.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_var.h,v
retrieving revision 1.44
diff -u -p -r1.44 in6_var.h
--- netinet6/in6_var.h 24 Oct 2013 11:31:43 -0000 1.44
+++ netinet6/in6_var.h 31 Oct 2013 13:58:39 -0000
@@ -64,6 +64,8 @@
#ifndef _NETINET6_IN6_VAR_H_
#define _NETINET6_IN6_VAR_H_
+#include <sys/task.h>
+
/*
* Interface address, Internet version. One of these structures
* is allocated for each interface with an Internet address.
@@ -480,6 +482,7 @@ do {
\
* belongs to.
*/
struct in6_multi_mship {
+ struct task i6mm_task; /* Allow network driver to sleep */
struct in6_multi *i6mm_maddr; /* Multicast address pointer */
LIST_ENTRY(in6_multi_mship) i6mm_chain; /* multicast options chain */
};
