Bug 272616
| Summary: | [panic] Reproducible kernel panic related to sendfile and IPSec | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Eugene Grosbein <eugen> | ||||
| Component: | kern | Assignee: | freebsd-net (Nobody) <net> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | ae, afedorov, franco, glebius, jhb, kib, markj | ||||
| Priority: | --- | Keywords: | crash | ||||
| Version: | 13.2-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| See Also: |
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=271393 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=271991 |
||||||
| Attachments: |
|
||||||
|
Description
Eugene Grosbein
2023-07-20 11:19:33 UTC
Adding more people to Cc: that may have an opinion. Some say m_unshare() should be extended to process mbufs with M_EXTPG, should it? m_unshare() is not enough. Really software IPSEC requires mapped mbufs. Even hw inline accel seems to need it, unfortunately. Try something like the attached patch. Created attachment 243503 [details]
ipsec: ensure that mbufs are mapped if ipsec is enabled
(In reply to Konstantin Belousov from comment #3) The patch did not apply to stable/13, so I applied it manually, rebuilt and reinstalled GENERIC and it really helped: no more panics even with default kern.ipc.mb_use_ext_pgs=1 I was too quick... Indeed, I cannot reproduce the panic with patched kernel, but the machine started to experience sudden resets without anything printed to serial console between "Login: " after boot and next BIOS POST messages: boot time Thu Jul 20 20:10 boot time Thu Jul 20 19:59 boot time Thu Jul 20 19:48 boot time Thu Jul 20 19:37 boot time Thu Jul 20 19:26 I switched to kernel.old for now. (In reply to Konstantin Belousov from comment #2) > Really software IPSEC requires mapped mbufs. Even hw inline accel seems to need it, unfortunately. Why is that? At least for sw it's only the payload that is unmapped, and crypto providers can handle that. (In reply to Mark Johnston from comment #6) By payload you mean mbuf data, right? IPSEC needs to match packet IP header against policy to decide should it do anything with it at all. Then it needs to select SA based on IP header, policy, and perhaps system defaults. All that requires access to the mbuf data. After the SA is selected, transformations are applied, which call into OCF. (In reply to Konstantin Belousov from comment #7) I mean, protocol headers (IP, TCP, etc.) are still mapped. More specifically, each mbuf in a chain can be mapped or not, and the IP header will generally be accessible even if the packet data is unmapped. (In reply to Mark Johnston from comment #8) Is it guaranteed that all protocol headers are mapped? Anyway, even quick look over the fundamental m_makespace() needed for ESP injection shows that it is not ready for unmapped mbufs. IMO. (In reply to Konstantin Belousov from comment #9) Well, there is no real guarantee, but if you only need to access the IP header, then mb_unmapped_to_ext() is overkill. In practice, protocol headers generated by the kernel will live in mapped mbufs that are separate from unmapped data. To be safer, we could introduce a mbuf function which guarantees that the first N bytes of the chain are mapped. m_makespace() needs a bit of work but fundamentally I don't see any problems with IPSec+unmapped mbufs. Really the bug here is that m_unshare() operates on the entire mbuf chain instead of stopping once we've gotten far enough to inject an IPSec header. (In reply to Mark Johnston from comment #10) Your reply is not much different from my evaluation: IPSEC needs complete audit to ensure that it works with unmapped pages in mbufs. Until this is done, either extpg should be administratively disabled, or a workaround used that I posted in the patch. (In reply to Konstantin Belousov from comment #11) I just wanted to establish the distinction between, "IPSec fundamentally cannot work with unmapped mbufs," and "IPSec is not yet ready to handle unmapped mbufs." I wasn't sure which one you meant with the initial comment. I think your patch is reasonable for 14.0. (In reply to Eugene Grosbein from comment #5) I realized that I recently enabled IPMI watchdog and our watchdogd(8) daemon but loaded ipmi.ko once manually not enabling its load at reboot, so booted with patched kernel and without ipmi.ko resulted in system reset by the watchdog every 10 minutes (and after switch to kernel.old, too). Fixed this pilot error. The main problem is that we don’t know where the mbuf will fly from with the M_EXTPG flag. Now it's an IPSEC, tomorrow something else. I think all functions that work with mbuf's should correctly handle unmapped mbuf's. But as a temporary patch, the solution proposed by kib@ is quite suitable. And m_unshare() should handle unmapped mbuf's correctly. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=bc310a95c58a3c570ed7e5103371453881e36ba1 commit bc310a95c58a3c570ed7e5103371453881e36ba1 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2023-07-20 12:08:24 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2023-07-21 18:51:13 +0000 ip output: ensure that mbufs are mapped if ipsec is enabled Ipsec needs access to packet headers to determine if a policy is applicable. It seems that typically IP headers are mapped, but the code is arguably needs to check this before blindly accessing them. Then, operations like m_unshare() and m_makespace() are not yet ready for unmapped mbufs. Ensure that the packet is mapped before calling into IPSEC_OUTPUT(). PR: 272616 Reviewed by: jhb, markj Sponsored by: NVidia networking MFC after: 1 week Differential revision: https://reviews.freebsd.org/D41112 sys/netinet/ip_output.c | 6 ++++++ sys/netinet6/ip6_output.c | 6 ++++++ 2 files changed, 12 insertions(+) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=7b335e9f690e77841e3eb7dbf3403429b10fe222 commit 7b335e9f690e77841e3eb7dbf3403429b10fe222 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2023-07-20 12:08:24 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2023-07-28 01:14:01 +0000 ip output: ensure that mbufs are mapped if ipsec is enabled PR: 272616 (cherry picked from commit bc310a95c58a3c570ed7e5103371453881e36ba1) sys/netinet/ip_output.c | 6 ++++++ sys/netinet6/ip6_output.c | 6 ++++++ 2 files changed, 12 insertions(+) Fixed in head and merged to stable/13. |