Allow carp configuration information to be supplied and retrieved via
netlink.
Sponsored by: Rubicon Communications, LLC ("Netgate")
Details
Details
- Reviewers
melifaro - Group Reviewers
network pfsense - Commits
- rG40e043596409: carp: add netlink interface
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 50321 Build 47213: arc lint + arc unit
Event Timeline
| lib/libifconfig/libifconfig_carp.c | ||
|---|---|---|
| 70 | We really need to put this in a common library or something, rather than repeat it everywhere we use NETLINK_GENERIC messages. | |
Comment Actions
Thank you for coming up with the netlink version! That was fast :-)
Generally LGTM, please see some comments inline.
I'll come up with an snl improvement diff today/tomorrow.
| lib/libifconfig/libifconfig_carp.c | ||
|---|---|---|
| 172–179 | I'll try to provide the iterator interface for multipart messages later on, as this pattern have to be repeated for each multipart message, which is tedious. | |
| 180 | Also: kernel often provides string error (available in e.error_str) | |
| 211 | Nit: this is not needed as it's the default flags for snl_create_msg_request(). | |
| 213 | Do you want to have it as a structure? | |
| 220 | Do we want to encode a name here an not ifindex? (and if we do, I'd suggest to have it as TLV, is it won't have IFNAMSIZ limit (and normally will consume less space). | |
| 221 | Nit: recording it as binary TLV allows to change CARP_KEY_LEN semi-easily. | |
| 229–239 | I'll wrap this into something like snl_request_sync(ss, hdr, e) | |
| sys/netinet/ip_carp.c | ||
| 2191 | Easier to get it as TLV, you'll get the size automatically (and don't even have to have a custom field parser) | |
| 2200 | Nit: would be easier to store it as TLV, then you'll get the provided size automatically. | |
| sys/netinet/ip_carp_nl.h | ||
| 41 | Personally, I'd provide interface referencing option both by interface name and interface index, but that's up to you :-) | |
| sys/netlink/netlink_snl.h | ||
| 454 ↗ | (On Diff #118728) | Ack, I'll add these (if you don't mind). |
Comment Actions
Use attributes (i.e. TLV) for everything.
I've gone with a single type enum for both get and set requests, because they
wound up being identical anyway.
| lib/libifconfig/libifconfig_carp.c | ||
|---|---|---|
| 180 | Is there an interface to provide that when implementing NETLINK_GENERIC? Right now the carp code returns errors by having the handler function return an error code. I'm probably not going to handle those either, because libifconfig also doesn't expect to deal with error strings. | |
| 213 | The thinking was that these are fields we're always going to want to supply, and we could add whatever extra we need as TLVs, but it's true that that sort of thing is hard to predict. | |
| 220 | Yeah, we should just use ifindex here. | |
| sys/netlink/netlink_snl.h | ||
| 454 ↗ | (On Diff #118728) | Sure, I can rebase easily enough. I've wound up only adding one on the snl / userspace side of things, and just (ab)using nlattr_get_uint32() in the kernel, because it takes void arguments, and sizeof(int32_t) == sizeof(uint32_t). I'm not sure it's worth separating those out on the kernel side. |
Comment Actions
I've raised D39092 with some snl(3) improvements that'll simplify the code.
| lib/libifconfig/libifconfig_carp.c | ||
|---|---|---|
| 70 | Done as snl_get_genl_family() | |
| 122 | Nit: #undef _OUT for consistency | |
| 123 | ||
| 138 | Nit: maybe it's worth adding handling interface as string & just use snl_add_msg_attr_u32(&nw, CARP_NL_IFNAME, name); | |
| 156 | ||
| 165 | Nit: you can skip checking the return values of snl_add_msg_attr_* and just check snl_finalize_msg() return. | |
| 175 | ||
| 180 | struct snl_errmsg_data e = {}; while ((hdr = snl_read_reply_multi(&ss, seq_id, &e)) != NULL) { ... } | |
| 180 | You can use nlmsg_report_err_msg() (or NLMSG_REPORT_ERR_MSG() which also does NLP_LOG(LOG_DEBUG)) to specify the error. Finally, you can add some attributes to the reply, using nlmsg_report_cookie(). | |
| sys/netinet/ip_carp.c | ||
| 2280 | ||
| 2412 | ||
| sys/netlink/netlink_snl.h | ||
| 735 ↗ | (On Diff #118807) | Whoops, looks like there are already signed versions - like snl_add_msg_attr_s32 |
| 454 ↗ | (On Diff #118728) | Done as snl_attr_get_int32(). |
Comment Actions
Generally Netlink part LGTM. Q: any concerns over not adding find-by-name handler? It seems cheap codeline-wise on the kernel side and will simplify client side (avoid if_getunit call).
| sys/netinet/ip_carp.c | ||
|---|---|---|
| 92 | That's netlink private header, it shouldn't be here. If something is required from that header, I'll look into making it a public interface. | |
| 2212 | Nit: maybe it's worth reusing set parser here? | |
| 2285–2291 | Nit: probably worth having this check as a separate function. | |
| 2295 | It's worth including NLM_F_MULTI to the reply's nlmsg_flags to indicate that this is potentially multipart message which's supposed to be ended by end_dump. | |
Comment Actions
I think I'd prefer to handle that in a separate commit. It's certainly a reasonable idea, but it's also something we can easily add afterwards.
| sys/netinet/ip_carp.c | ||
|---|---|---|
| 92 | I need access to the struct ucred in nl_pstate->nlp->nl_cred for credentials checking in the get call, which required having the definition of struct nlpcb. Presumably the thing to do is to add a simple accessor function to the public API. Something like: diff --git a/sys/netlink/netlink_message_parser.c b/sys/netlink/netlink_message_parser.c index e30138266d9d..bed26d04407f 100644 --- a/sys/netlink/netlink_message_parser.c +++ b/sys/netlink/netlink_message_parser.c @@ -51,6 +51,12 @@ __FBSDID("$FreeBSD$"); #include <netlink/netlink_debug.h> _DECLARE_DEBUG(LOG_DEBUG); +struct ucred * +npt_get_cred(struct nl_pstate *npt) +{ + return (npt->nlp->nl_cred); +} + bool nlmsg_report_err_msg(struct nl_pstate *npt, const char *fmt, ...) { diff --git a/sys/netlink/netlink_message_parser.h b/sys/netlink/netlink_message_parser.h index 083c93dcbf8d..1ead2652dd53 100644 --- a/sys/netlink/netlink_message_parser.h +++ b/sys/netlink/netlink_message_parser.h @@ -82,6 +82,8 @@ npt_alloc(struct nl_pstate *npt, int len) } #define npt_alloc_sockaddr(_npt, _len) ((struct sockaddr *)(npt_alloc(_npt, _len))) +struct ucred *npt_get_cred(struct nl_pstate *npt); + typedef int parse_field_f(void *hdr, struct nl_pstate *npt, void *target); struct nlfield_parser { (And I've noticed we can remove that argument from carp_ioctl_set(), which led me to spot the missing check in the old ioctl, so let's fix that now. | |
Comment Actions
Ack, no concerns here.
Also, you marked some comments as done but the review was not updated - just wanted to check if we're on the same page w.r.t some of the changes (I'm mostly curious about get/set parsers) :-)
| sys/netinet/ip_carp.c | ||
|---|---|---|
| 92 | Makes sense. I committed slightly different version in 046acc2bfd13. | |
Comment Actions
Nit: it'll break the build w/o NETLINK. I'll come up with a diff similar to D39148 in a day or two.
Comment Actions
That's going to be an annoying once once the unicast support lands. You'll have to make bits of that conditional on NETLINK then, because there's no interface to configure it without it.
And I do think the reasonable path is to just not have carp unicast without NETLINK. Happily you can leave most of the kernel bits that are not directly NETLINK related in, because it does still default to doing multicast.
Comment Actions
Currently I'm trying the following approach:
- have netlink_glue.c, which will define all necessary functions & parsers, to make the stuff compile correctly (and load genetlink modules).
- most functions (when compiled as a module) will be implemented as the stubs, with the ability to switch to the actual implementation once the module is loaded.
From the netlink user standpoint it should be transparent. Maybe it will be enough to address the space/other concerns.
If not, I'll have to implement NETLINK_SUPPORT conditional to guard netlink-enabled module parts.
I'll hopefully come up with a review today/tomorrow.
Comment Actions
Hi all,
I hope it is OK that I write this here, if not please tell me what will be a better place to do it.
This commit has made the ENA driver crash upon kldunload if_ena.ko. (I made sure by running buildworld on this commit and on the commit before it)
Here is the full call stack that was dumped:
curthread () at /root/freebsd-src/sys/amd64/include/pcpu_aux.h:59
59 asm("movq %%gs:%P1,%0" : "=r" (td) : "n" (offsetof(struct pcpu,
(kgdb) #0 __curthread () at /root/freebsd-src/sys/amd64/include/pcpu_aux.h:59
#1 doadump (textdump=textdump@entry=1)
at /root/freebsd-src/sys/kern/kern_shutdown.c:407
#2 0xffffffff80bedc6c in kern_reboot (howto=260)
at /root/freebsd-src/sys/kern/kern_shutdown.c:528
#3 0xffffffff80bee18f in vpanic (fmt=<optimized out>,
ap=ap@entry=0xfffffe01fef62ae0)
at /root/freebsd-src/sys/kern/kern_shutdown.c:972
#4 0xffffffff80bedf13 in panic (fmt=<unavailable>)
at /root/freebsd-src/sys/kern/kern_shutdown.c:896
#5 0xffffffff810e2b39 in trap_fatal (frame=0xfffffe01fef62b70, eva=0)
at /root/freebsd-src/sys/amd64/amd64/trap.c:954
#6 <signal handler called>
#7 dump_sa (nw=nw@entry=0xfffffe01fef62d08, attr=attr@entry=1,
sa=0xdeadc0dedeadc0de) at /root/freebsd-src/sys/netlink/route/iface.c:210
#8 0xffffffff80e5659a in dump_iface (nw=nw@entry=0xfffffe01fef62d08,
ifp=ifp@entry=0xfffff80109bbe800, hdr=hdr@entry=0xfffffe01fef62d48,
if_flags_mask=if_flags_mask@entry=0)
at /root/freebsd-src/sys/netlink/route/iface.c:279
#9 0xffffffff80e55e7b in rtnl_handle_ifevent (ifp=0xfffff80109bbe800,
nlmsg_type=<optimized out>, if_flags_mask=0)
at /root/freebsd-src/sys/netlink/route/iface.c:943
#10 0xffffffff80d1fc1d in do_link_state_change (arg=0xfffff80109bbe800,
pending=1) at /root/freebsd-src/sys/net/if.c:2205
#11 0xffffffff80c5233a in taskqueue_run_locked (
queue=queue@entry=0xfffff80106ce7100)
at /root/freebsd-src/sys/kern/subr_taskqueue.c:514
#12 0xffffffff80c5224d in taskqueue_run (queue=0xfffff80106ce7100)
at /root/freebsd-src/sys/kern/subr_taskqueue.c:529
#13 0xffffffff80ba8126 in intr_event_execute_handlers (ie=0xfffff80106a9d300,
p=<optimized out>) at /root/freebsd-src/sys/kern/kern_intr.c:1207
#14 ithread_execute_handlers (ie=0xfffff80106a9d300, p=<optimized out>)
at /root/freebsd-src/sys/kern/kern_intr.c:1220
#15 ithread_loop (arg=arg@entry=0xfffff80106c951c0)
at /root/freebsd-src/sys/kern/kern_intr.c:1308
#16 0xffffffff80ba45c0 in fork_exit (
callout=0xffffffff80ba7eb0 <ithread_loop>, arg=0xfffff80106c951c0,
frame=0xfffffe01fef62f40) at /root/freebsd-src/sys/kern/kern_fork.c:1102
#17 <signal handler called>
(kgdb)
I don't have much experience with CARP, and I was wondering if anything obvious jumps out from this call stack with regard to this commit?
Thanks!
Arthur
Comment Actions
I've write to you and CC @melifaro and @kp .
The FreeBSD Phabricator is for code review / collaboration.
For bugs you may want to report to https://bugs.freebsd.org or mailing list (freebsd-current@FreeBSD.org in this context).
Comment Actions
Thank you for the report!
I agree with the assessment, it's not carp related.
When the ifnet departure hook is called, indeed some stuff from the ifnet may have been already disappeared. I'll update the callback handler to use only basic interface info, that should be present till the end.
Thanks!
Arthur
Comment Actions
Thanks @melifaro
I opened https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=270813 with all the details of reproduction steps and my investigation.
Thanks!
Arthur