Page MenuHomeFreeBSD
Differential D4522

inet6: Do not assume every interface has ip6 enabled
ClosedPublic
Actions

Authored by kp on Dec 11 2015, 7:49 PM.
Tags
Referenced Files
Unknown Object (File)
Mon, Apr 7, 5:12 PM
Unknown Object (File)
Feb 1 2025, 5:42 AM
Unknown Object (File)
Jan 24 2025, 1:16 PM
Unknown Object (File)
Jan 20 2025, 5:25 AM
Unknown Object (File)
Jan 1 2025, 9:55 PM
Unknown Object (File)
Dec 27 2024, 1:44 PM
Unknown Object (File)
Dec 7 2024, 3:16 PM
Unknown Object (File)
Nov 21 2024, 3:22 PM
View All 97 Files
Subscribers
admin_support.od.ua
imp
melifaro

Details

Reviewers
melifaro
hrs
Group Reviewers
network
Commits
rS292219: inet6: Do not assume every interface has ip6 enabled.
Summary

Certain interfaces (e.g. pfsync0) do not have ip6 addresses (in other words,
ifp->if_afdata[AF_INET6] is NULL). Ensure we don't panic when the MTU is
updated.

pfsync interfaces will never have ip6 support, because it's explicitly disabled
in in6_domifattach().

PR: 205194

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kp updated this revision to Diff 11143.Dec 11 2015, 7:49 PM
kp retitled this revision from to inet6: Do not assume every interface has ip6 enabled.
kp updated this object.
kp edited the test plan for this revision. (Show Details)
kp set the repository for this revision to rS FreeBSD src repository - subversion.
kp added a project: network.
Herald added a subscriber: imp. · View Herald TranscriptDec 11 2015, 7:49 PM
admin_support.od.ua added a subscriber: admin_support.od.ua.Dec 11 2015, 8:34 PM
melifaro added a subscriber: melifaro.Dec 11 2015, 8:50 PM
melifaro accepted this revision.Dec 11 2015, 9:13 PM
melifaro added a reviewer: melifaro.

According to the summary, it looks ok. Ensuring entire inet6/ code would work in all similar cases require (probably much) more work :)

sys/netinet6/in6.c
2427

Well, the function is supposed to return MTU value. 46 is definitely better than panic, but It would probably be better to return ifp->if_mtu.

This revision is now accepted and ready to land.Dec 11 2015, 9:13 PM
melifaro added a reviewer: network.Dec 11 2015, 9:13 PM
kp added inline comments.Dec 11 2015, 10:14 PM
sys/netinet6/in6.c
2427

You're quite right. We should do what the caller (if_getmtu_family()) does if it can't get a better answer and return ifp->if_mtu.

kp updated this revision to Diff 11150.Dec 11 2015, 10:17 PM
kp updated this object.
kp edited edge metadata.
This revision now requires review to proceed.Dec 11 2015, 10:17 PM
melifaro accepted this revision.Dec 14 2015, 9:25 AM
melifaro edited edge metadata.
This revision is now accepted and ready to land.Dec 14 2015, 9:25 AM
hrs accepted this revision.Dec 14 2015, 9:30 AM
hrs added a reviewer: hrs.
Closed by commit rS292219: inet6: Do not assume every interface has ip6 enabled. (authored by kp). · Explain WhyDec 14 2015, 7:45 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
netinet6/
2 lines
2 lines

Diff 11143

View Options

sys/netinet6/in6.c

Show First 20 Lines • Show All 727 Lines • ▼ Show 20 Lines if (IN_LOOPBACK(ntohl(ip->ip_dst.s_addr)) ||
IN_LOOPBACK(ntohl(ip->ip_src.s_addr))) { IN_LOOPBACK(ntohl(ip->ip_src.s_addr))) {
if ((ifp->if_flags & IFF_LOOPBACK) == 0) { if ((ifp->if_flags & IFF_LOOPBACK) == 0) {
IPSTAT_INC(ips_badaddr); IPSTAT_INC(ips_badaddr);
error = EADDRNOTAVAIL; error = EADDRNOTAVAIL;
goto bad; goto bad;
} }
} }
m->m_pkthdr.csum_flags |= CSUM_IP; /* Ensure the packet data is mapped if the interface requires it. */
if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA & ~ifp->if_hwassist) { if ((ifp->if_capenable & IFCAP_MEXTPG) == 0) {
m = mb_unmapped_to_ext(m); m = mb_unmapped_to_ext(m);
if (m == NULL) { if (m == NULL) {
IPSTAT_INC(ips_odropped); IPSTAT_INC(ips_odropped);
error = ENOBUFS; error = ENOBUFS;
goto bad; goto bad;
} }
}
m->m_pkthdr.csum_flags |= CSUM_IP;
if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA & ~ifp->if_hwassist) {
in_delayed_cksum(m); in_delayed_cksum(m);
m->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA; m->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA;
} else if ((ifp->if_capenable & IFCAP_MEXTPG) == 0) {
jhbUnsubmitted
Done
Inline Actions

This is now broken for an interface that doesn't support IFCAP_MEXTPG and has checksum offload disabled as we will not fall through to the else. I think the IFCAP_MEXTPG check now needs to just be its own check. If it makes the checksum calculations more optimal we should perhaps do it before doing the checksums.

jhb: This is now broken for an interface that doesn't support IFCAP_MEXTPG and has checksum offload…
markjAuthorUnsubmitted
Done
Inline Actions

Oops, thanks. I realized this when writing the patch but forgot to actually do something about it. :(

I guess m_apply() is a bit cheaper for non-extpg mbufs, so it makes some sense to do that first.

markj: Oops, thanks. I realized this when writing the patch but forgot to actually do something about…
m = mb_unmapped_to_ext(m);
if (m == NULL) {
IPSTAT_INC(ips_odropped);
error = ENOBUFS;
goto bad;
} }
}
#if defined(SCTP) || defined(SCTP_SUPPORT) #if defined(SCTP) || defined(SCTP_SUPPORT)
if (m->m_pkthdr.csum_flags & CSUM_SCTP & ~ifp->if_hwassist) { if (m->m_pkthdr.csum_flags & CSUM_SCTP & ~ifp->if_hwassist) {
sctp_delayed_cksum(m, (uint32_t)(ip->ip_hl << 2)); sctp_delayed_cksum(m, (uint32_t)(ip->ip_hl << 2));
m->m_pkthdr.csum_flags &= ~CSUM_SCTP; m->m_pkthdr.csum_flags &= ~CSUM_SCTP;
} }
#endif #endif
/* /*
▲ Show 20 Lines • Show All 127 Lines • ▼ Show 20 Linesip_fragment(struct ip *ip, struct mbuf **m_frag, int mtu,
if (len < 8) if (len < 8)
return EMSGSIZE; return EMSGSIZE;
/* /*
* If the interface will not calculate checksums on * If the interface will not calculate checksums on
* fragmented packets, then do it here. * fragmented packets, then do it here.
*/ */
if (m0->m_pkthdr.csum_flags & CSUM_DELAY_DATA) { if (m0->m_pkthdr.csum_flags & CSUM_DELAY_DATA) {
m0 = mb_unmapped_to_ext(m0);
if (m0 == NULL) {
error = ENOBUFS;
IPSTAT_INC(ips_odropped);
goto done;
}
in_delayed_cksum(m0); in_delayed_cksum(m0);
m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA; m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA;
} }
#if defined(SCTP) || defined(SCTP_SUPPORT) #if defined(SCTP) || defined(SCTP_SUPPORT)
if (m0->m_pkthdr.csum_flags & CSUM_SCTP) { if (m0->m_pkthdr.csum_flags & CSUM_SCTP) {
sctp_delayed_cksum(m0, hlen); sctp_delayed_cksum(m0, hlen);
m0->m_pkthdr.csum_flags &= ~CSUM_SCTP; m0->m_pkthdr.csum_flags &= ~CSUM_SCTP;
} }
▲ Show 20 Lines • Show All 703 Lines • Show Last 20 Lines
View Options

sys/netinet6/nd6.c

Show First 20 Lines • Show All 727 Lines • ▼ Show 20 Lines if (IN_LOOPBACK(ntohl(ip->ip_dst.s_addr)) ||
IN_LOOPBACK(ntohl(ip->ip_src.s_addr))) { IN_LOOPBACK(ntohl(ip->ip_src.s_addr))) {
if ((ifp->if_flags & IFF_LOOPBACK) == 0) { if ((ifp->if_flags & IFF_LOOPBACK) == 0) {
IPSTAT_INC(ips_badaddr); IPSTAT_INC(ips_badaddr);
error = EADDRNOTAVAIL; error = EADDRNOTAVAIL;
goto bad; goto bad;
} }
} }
m->m_pkthdr.csum_flags |= CSUM_IP; /* Ensure the packet data is mapped if the interface requires it. */
if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA & ~ifp->if_hwassist) { if ((ifp->if_capenable & IFCAP_MEXTPG) == 0) {
m = mb_unmapped_to_ext(m); m = mb_unmapped_to_ext(m);
if (m == NULL) { if (m == NULL) {
IPSTAT_INC(ips_odropped); IPSTAT_INC(ips_odropped);
error = ENOBUFS; error = ENOBUFS;
goto bad; goto bad;
} }
}
m->m_pkthdr.csum_flags |= CSUM_IP;
if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA & ~ifp->if_hwassist) {
in_delayed_cksum(m); in_delayed_cksum(m);
m->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA; m->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA;
} else if ((ifp->if_capenable & IFCAP_MEXTPG) == 0) {
jhbUnsubmitted
Done
Inline Actions

This is now broken for an interface that doesn't support IFCAP_MEXTPG and has checksum offload disabled as we will not fall through to the else. I think the IFCAP_MEXTPG check now needs to just be its own check. If it makes the checksum calculations more optimal we should perhaps do it before doing the checksums.

jhb: This is now broken for an interface that doesn't support IFCAP_MEXTPG and has checksum offload…
markjAuthorUnsubmitted
Done
Inline Actions

Oops, thanks. I realized this when writing the patch but forgot to actually do something about it. :(

I guess m_apply() is a bit cheaper for non-extpg mbufs, so it makes some sense to do that first.

markj: Oops, thanks. I realized this when writing the patch but forgot to actually do something about…
m = mb_unmapped_to_ext(m);
if (m == NULL) {
IPSTAT_INC(ips_odropped);
error = ENOBUFS;
goto bad;
} }
}
#if defined(SCTP) || defined(SCTP_SUPPORT) #if defined(SCTP) || defined(SCTP_SUPPORT)
if (m->m_pkthdr.csum_flags & CSUM_SCTP & ~ifp->if_hwassist) { if (m->m_pkthdr.csum_flags & CSUM_SCTP & ~ifp->if_hwassist) {
sctp_delayed_cksum(m, (uint32_t)(ip->ip_hl << 2)); sctp_delayed_cksum(m, (uint32_t)(ip->ip_hl << 2));
m->m_pkthdr.csum_flags &= ~CSUM_SCTP; m->m_pkthdr.csum_flags &= ~CSUM_SCTP;
} }
#endif #endif
/* /*
▲ Show 20 Lines • Show All 127 Lines • ▼ Show 20 Linesip_fragment(struct ip *ip, struct mbuf **m_frag, int mtu,
if (len < 8) if (len < 8)
return EMSGSIZE; return EMSGSIZE;
/* /*
* If the interface will not calculate checksums on * If the interface will not calculate checksums on
* fragmented packets, then do it here. * fragmented packets, then do it here.
*/ */
if (m0->m_pkthdr.csum_flags & CSUM_DELAY_DATA) { if (m0->m_pkthdr.csum_flags & CSUM_DELAY_DATA) {
m0 = mb_unmapped_to_ext(m0);
if (m0 == NULL) {
error = ENOBUFS;
IPSTAT_INC(ips_odropped);
goto done;
}
in_delayed_cksum(m0); in_delayed_cksum(m0);
m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA; m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA;
} }
#if defined(SCTP) || defined(SCTP_SUPPORT) #if defined(SCTP) || defined(SCTP_SUPPORT)
if (m0->m_pkthdr.csum_flags & CSUM_SCTP) { if (m0->m_pkthdr.csum_flags & CSUM_SCTP) {
sctp_delayed_cksum(m0, hlen); sctp_delayed_cksum(m0, hlen);
m0->m_pkthdr.csum_flags &= ~CSUM_SCTP; m0->m_pkthdr.csum_flags &= ~CSUM_SCTP;
} }
▲ Show 20 Lines • Show All 703 Lines • Show Last 20 Lines