netlink: Implement support for netlink sockets #69
Conversation
This change imports netlink related Linux UAPI headers, used to provide basic netlink functionality. Minor changes to upstream files for the unikraft build system. Source of import: Repository: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git Tag: v5.10 (2c85ebc57b) Path: include/uapi/linux/* Signed-off-by: Simon Kuenzer <[email protected]>
This change introduces support for using AF_NETLINK sockets to talk to the lwip stack, supporting RTM_GETLINK and RTM_GETADDR operations. Co-authored-by: Simon Kuenzer <[email protected]> Signed-off-by: Simon Kuenzer <[email protected]> Signed-off-by: Andrei Tatar <[email protected]>
mogasergiu
left a comment
mogasergiu
left a comment
There was a problem hiding this comment.
LGTM in general. Though one quick question. I see several nlbuf_allocs in this file but no nlbuf_frees. Do they ever get freed by something?
|
|
||
| uk_pr_debug("Reply with error msg: %d\n", err); | ||
|
|
||
| nlh = nlbuf_reserve(sb, NLA_HDRLEN); |
There was a problem hiding this comment.
I think this should've been NLMSG_HDRLEN?
There was a problem hiding this comment.
This is using the align-padded version on purpose, since the header will be followed by a payload, which requires alignment. The netlink header lays this out.
But thanks for mentioning it, I now see that nlmsg_len and other allocs within the file are a bit off; will fix.
| nlh->nlmsg_flags = flags; | ||
| nlh->nlmsg_pid = nl_ctx_pid(ctx); | ||
| nlh->nlmsg_seq = req->nlmsg_seq; | ||
| nlh->nlmsg_len = nlbuf_len(sb); |
There was a problem hiding this comment.
Should this be called last since nlbuf_reserve below might update the len?
There was a problem hiding this comment.
Indeed, thanks for catching; will update.
| #else /* !LWIP_IPV6*/ | ||
| ip4_addr_2_in_addr(&netif->ip_addr, inbc); | ||
| #endif /* !LWIP_IPV6 */ | ||
| *inbc ^= in_addr_netmask(ifa->ifa_prefixlen); |
There was a problem hiding this comment.
For broadcast shouldn't this be *inbc |= ~in_addr_netmask(ifa->ifa_prefixlen);
There was a problem hiding this comment.
Indeed it should; thanks 👍 .
| if (unlikely(!rep)) | ||
| return -ENOMEM; | ||
|
|
||
| reply_err(ctx, rep, -ENOTSUP, 0, req); |
There was a problem hiding this comment.
tbh, ENOTSUP is also a thing and they're synonyms :P
|
|
||
| /* Both data structures are in network byte order */ | ||
| /* Both data structures consists of a __u32 value */ | ||
| *((__u32 *)out) = (__u32) in->addr; |
There was a problem hiding this comment.
I see this in other places in this patch as well. We should be consistent and not have whitespaces between type cast and the variable. Also, I just noticed, but why are we using kernel internal types in this external library port patch? Some functions use them others don't.
| uk_pr_debug("nlh->pid: %"__PRIu32"\n", nlh->nlmsg_pid); | ||
| } | ||
|
|
||
| static size_t netif_name_len(struct netif *ni) |
| } | ||
|
|
||
| #if MIB2_STATS | ||
| static inline __bool netif_is_loopback(struct netif *netif) |
|
|
||
| #define NLMSG_ERR_LEN NLMSG_LENGTH(sizeof(struct nlmsgerr)) | ||
|
|
||
| static void reply_err(struct nl_ctx *ctx, struct uk_streambuf *sb, |
There was a problem hiding this comment.
Ditto for ctx? Ok, I see other places in this file don't have const for some arguments that seemingly could have it. Maybe I am missing something but probably worth taking a look.
4 participants
This changeset allows lwip to respond to userspace requests for interfaces and configured addresses via netlink sockets.
Based off work by @skuenzer .
Requires the following unikraft core PR:
unikraft/unikraft#1736