Re: kernel diagnostic assertion "ifa == rt->rt_ifa", rt_ifa_addlocal
On Sat, Dec 17, 2016 at 02:33:37AM +0100, Alexander Bluhm wrote:
> Hi,
>
> If rt_ifa_addlocal() in in_ifinit() fails, the address has been
> added to the interface address list, but the local route is missing.
> This inconsistency can result in a panic later.
>
> panic: kernel diagnostic assertion "ifa == rt->rt_ifa" failed: file
> "/crypt/home/bluhm/openbsd/cvs/src/sys/netinet/if_ether.c", line 206
>
> I have seen this crash in production on a 5.9 system and could
> reproduce it on -current by inducing an error in rt_ifa_addlocal().
>
> So in case of an error, remove the interface address to get a
> consistent state again.
>
> ok?
OK.
The little dance with an aditional error variable is a bit confusing.
It would be nice to avoid the extra variable. But off-hand I could not
find a simpler diff which keeps the semantics of re-adding the original
address if the ioctl errors. So your approach seems fine.
>
> bluhm
>
> Index: netinet/in.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v
> retrieving revision 1.130
> diff -u -p -u -p -r1.130 in.c
> --- netinet/in.c 5 Dec 2016 15:31:43 - 1.130
> +++ netinet/in.c 17 Dec 2016 01:27:30 -
> @@ -603,7 +603,7 @@ in_ifinit(struct ifnet *ifp, struct in_i
> {
> u_int32_t i = sin->sin_addr.s_addr;
> struct sockaddr_in oldaddr;
> - int error = 0;
> + int error = 0, rterror;
>
> splsoftassert(IPL_SOFTNET);
>
> @@ -633,10 +633,16 @@ in_ifinit(struct ifnet *ifp, struct in_i
>* error occured, put back the original address.
>*/
> ifa_add(ifp, >ia_ifa);
> - rt_ifa_addlocal(>ia_ifa);
> + rterror = rt_ifa_addlocal(>ia_ifa);
>
> if (error)
> goto out;
> + if (rterror) {
> + if (!newaddr)
> + ifa_del(ifp, >ia_ifa);
> + error = rterror;
> + goto out;
> + }
>
> if (ia->ia_netmask == 0) {
> if (IN_CLASSA(i))
>
kernel diagnostic assertion "ifa == rt->rt_ifa", rt_ifa_addlocal
Hi,
If rt_ifa_addlocal() in in_ifinit() fails, the address has been
added to the interface address list, but the local route is missing.
This inconsistency can result in a panic later.
panic: kernel diagnostic assertion "ifa == rt->rt_ifa" failed: file
"/crypt/home/bluhm/openbsd/cvs/src/sys/netinet/if_ether.c", line 206
I have seen this crash in production on a 5.9 system and could
reproduce it on -current by inducing an error in rt_ifa_addlocal().
So in case of an error, remove the interface address to get a
consistent state again.
ok?
bluhm
Index: netinet/in.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v
retrieving revision 1.130
diff -u -p -u -p -r1.130 in.c
--- netinet/in.c5 Dec 2016 15:31:43 - 1.130
+++ netinet/in.c17 Dec 2016 01:27:30 -
@@ -603,7 +603,7 @@ in_ifinit(struct ifnet *ifp, struct in_i
{
u_int32_t i = sin->sin_addr.s_addr;
struct sockaddr_in oldaddr;
- int error = 0;
+ int error = 0, rterror;
splsoftassert(IPL_SOFTNET);
@@ -633,10 +633,16 @@ in_ifinit(struct ifnet *ifp, struct in_i
* error occured, put back the original address.
*/
ifa_add(ifp, >ia_ifa);
- rt_ifa_addlocal(>ia_ifa);
+ rterror = rt_ifa_addlocal(>ia_ifa);
if (error)
goto out;
+ if (rterror) {
+ if (!newaddr)
+ ifa_del(ifp, >ia_ifa);
+ error = rterror;
+ goto out;
+ }
if (ia->ia_netmask == 0) {
if (IN_CLASSA(i))
Diagnosed problem with GPT / NVMe - need guidance
Hi tech@, I had posted earlier about trying to dual boot Arch Linux and OpenBSD-current on a new Thinkpad X1 Carbon (4th generation) with a 1 TB NVMe drive. The problem I had was that when I set up things from Arch first, the OpenBSD fdisk and disklabel utilities acted like they could only see a single large partition spanning the entire drive. I decided to go at this from another direction. I backed up my data and wiped the drive, figuring that if I started out with OpenBSD, I'd be ok. Here's where it gets weird. I created the partitions (UEFI and OpenBSD) from fdisk in GPT mode, then used disklabel to create the filesystems. Everything was fine. I booted up the Arch installer, launched gdisk and immediate got errors. Specifically: Warning! Disk size is smaller than the main header indicates! Loading secondary header from the last sector of the disk! You should use ‘v’ to verify disk integrity, and perhaps options on the expert’s menu to repair the disk. - and - Caution: Invalid backup GPT header, but valid main header; regenerating backup header from main header. - and - Warning! Main and backup partition tables differ! Use the ‘c’ and ‘e’ options on the recovery & transformation menu to examine the two tables. - and - Warning! One or more CRCs don’t match. You should repair the disk! I thought that maybe the secondary GPT was messed up so I went ahead and repaired things on Linux using the primary and recreating the secondary. I installed Arch and then rebooted to OpenBSD. Kernel panic because it couldn't find the filesystem. I tried this several times and got the same result. Finally, I partitioned the disk as MBR and everything worked just fine. Clearly there is some subtlety with GPT, the NVMe driver and/or the size of the disk (1TB Samsung). Any ideas? What data could I gather to help diagnose this? Thanks!
Re: igmp: set rtableid on new mbufs
On Wed, Dec 14, 2016 at 06:59:42PM +0100, Martin Pieuchot wrote:
> On 14/12/16(Wed) 16:54, Rafael Zalamena wrote:
> > After running the igmpproxy in multiple domains I noticed that the kernel
> > started complaining about sending packets on wrong domains. Here is the
> > exact message:
> > "
> > vio1: trying to send packet on wrong domain. if 1 vs. mbuf 0
> > "
> >
> > After some debugging I traced the problem to the igmp_sendpkt() function
> > and it seems that it is missing to set the mbuf rdomain, so this is
> > exactly what this diff does.
>
> It doesn't make sense to call if_get(9) when all the callers of
> igmp_sendpkt() already have a reference to the sending ifp. if_get(9)
> has a cost and adds complexity. I'd rather pass ifp or the rdomain to
> igmp_sendpkt().
Following mpi@'s suggestion here is a new diff that removes the
if_get()/if_put() from igmp_sendpkt() and make it the callers
responsability.
ok?
Index: sys/netinet/igmp.c
===
RCS file: /home/obsdcvs/src/sys/netinet/igmp.c,v
retrieving revision 1.57
diff -u -p -r1.57 igmp.c
--- sys/netinet/igmp.c 14 Dec 2016 17:15:56 - 1.57
+++ sys/netinet/igmp.c 16 Dec 2016 11:19:42 -
@@ -104,7 +104,7 @@ static struct mbuf *router_alert;
struct igmpstat igmpstat;
void igmp_checktimer(struct ifnet *);
-void igmp_sendpkt(struct in_multi *, int, in_addr_t);
+void igmp_sendpkt(struct ifnet *, struct in_multi *, int, in_addr_t);
int rti_fill(struct in_multi *);
struct router_info * rti_find(struct ifnet *);
void igmp_input_if(struct ifnet *, struct mbuf *, int);
@@ -509,7 +509,7 @@ igmp_joingroup(struct in_multi *inm)
if ((i = rti_fill(inm)) == -1)
goto out;
- igmp_sendpkt(inm, i, 0);
+ igmp_sendpkt(ifp, inm, i, 0);
inm->inm_state = IGMP_DELAYING_MEMBER;
inm->inm_timer = IGMP_RANDOM_DELAY(
IGMP_MAX_HOST_REPORT_DELAY * PR_FASTHZ);
@@ -534,7 +534,8 @@ igmp_leavegroup(struct in_multi *inm)
if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) &&
ifp && (ifp->if_flags & IFF_LOOPBACK) == 0)
if (inm->inm_rti->rti_type != IGMP_v1_ROUTER)
- igmp_sendpkt(inm, IGMP_HOST_LEAVE_MESSAGE,
+ igmp_sendpkt(ifp, inm,
+ IGMP_HOST_LEAVE_MESSAGE,
INADDR_ALLROUTERS_GROUP);
break;
case IGMP_LAZY_MEMBER:
@@ -582,10 +583,10 @@ igmp_checktimer(struct ifnet *ifp)
} else if (--inm->inm_timer == 0) {
if (inm->inm_state == IGMP_DELAYING_MEMBER) {
if (inm->inm_rti->rti_type == IGMP_v1_ROUTER)
- igmp_sendpkt(inm,
+ igmp_sendpkt(ifp, inm,
IGMP_v1_HOST_MEMBERSHIP_REPORT, 0);
else
- igmp_sendpkt(inm,
+ igmp_sendpkt(ifp, inm,
IGMP_v2_HOST_MEMBERSHIP_REPORT, 0);
inm->inm_state = IGMP_IDLE_MEMBER;
}
@@ -611,22 +612,17 @@ igmp_slowtimo(void)
}
void
-igmp_sendpkt(struct in_multi *inm, int type, in_addr_t addr)
+igmp_sendpkt(struct ifnet *ifp, struct in_multi *inm, int type,
+in_addr_t addr)
{
- struct ifnet *ifp;
struct mbuf *m;
struct igmp *igmp;
struct ip *ip;
struct ip_moptions imo;
- if ((ifp = if_get(inm->inm_ifidx)) == NULL)
- return;
-
MGETHDR(m, M_DONTWAIT, MT_HEADER);
- if (m == NULL) {
- if_put(ifp);
+ if (m == NULL)
return;
- }
/*
* Assume max_linkhdr + sizeof(struct ip) + IGMP_MINLEN
@@ -674,7 +670,6 @@ igmp_sendpkt(struct in_multi *inm, int t
#endif /* MROUTING */
ip_output(m, router_alert, NULL, IP_MULTICASTOPTS, , NULL, 0);
- if_put(ifp);
++igmpstat.igps_snd_reports;
}
