nat46-module: Add support for Netlink sockets#8
nat46-module: Add support for Netlink sockets#8srsmiraato wants to merge 1 commit intoayourtch:masterfrom
Conversation
The module uses procfs for communication between the user and kernel space. Add support to use Netlink, and updated README.md for instructions to enable this support.
ayourtch
left a comment
ayourtch
left a comment
There was a problem hiding this comment.
Overall question: "why?" Netlink/ioctl-type configuration is traditionally done with the structs, with userspace tools parsing the data, and that is the whole point of it, and why I did not do it. This patch just moves the same string-based API into netlink - why ?
| #endif | ||
|
|
||
| #ifdef PROTO_NETLINK | ||
| #define NETLINK_USER 31 |
There was a problem hiding this comment.
What is the source of the magic value "31" ? Does the code work if you change it "42" here ? If not - it is probably defined somewhere as 31, so that file would need to be included instead of the #define here...
There was a problem hiding this comment.
Thanks for the feedback. I'll keep your suggestion in mind as I update my patch.
| struct netlink_kernel_cfg nl_cfg = { | ||
| .input = nat46_nl_recv_msg | ||
| }; | ||
| printk("nat46: module (version %s) loaded.\n", NAT46_VERSION); |
There was a problem hiding this comment.
Two places printing the exact same content is confusing. If anything, this place should print something about procfs support not present, please use netlink.
There was a problem hiding this comment.
Thanks for the feedback. I'll keep your suggestion in mind as I update my patch.
| nl_sk = netlink_kernel_create(&init_net, NETLINK_USER, &nl_cfg); | ||
| if (!nl_sk) | ||
| { | ||
| printk(KERN_ALERT "nat46: error creating socket\n"); |
There was a problem hiding this comment.
Should be more descriptive. Mention that it is a netlink control socket.
There was a problem hiding this comment.
Thanks for the feedback. I'll keep your suggestion in mind as I update my patch.
| pid = nl_hdr->nlmsg_pid; | ||
| cmd = (char *) nlmsg_data (nl_hdr); | ||
|
|
||
| nat46_proc_cmd(cmd); |
There was a problem hiding this comment.
nat46_proc_cmd assumes a zero-terminated string which it is always the case for the procfs case. Here the code takes an arbitrary pointer from the message and passes it to the string parsing function without validation. You have to make sure here that the user can trigger the parsing code go beyond the boundary of the cmd.
Also, the whole point of using the netlink and similar structures is to have the parsing of the data done in the user space, and supply well-defined data structures to the kernel space. So - could you describe a little, what is the rationale to use the netlink to pass the strings vs. just using procfs ?
There was a problem hiding this comment.
Our system uses Netlink as the interface between kernel and user space, so I thought of having a Netlink socket support for this module to keep things consistent. Thanks for your feedback. I'll keep your suggestions in mind as I update my patch.
| To use Netlink sockets instead, add the following to nat46/nat46/modules/Makefile | ||
| when compiling: | ||
| ``` | ||
| EXTRA_CFLAGS += -DPROTO_NETLINK |
There was a problem hiding this comment.
The define should be a bit more specific for NAT46 module and more self-descriptive. NAT46_USE_NETLINK_CONFIG or something like that.
There was a problem hiding this comment.
Thanks for your feedback. I'll keep your suggestion in mind as I update my patch.
2 participants
The module uses procfs for communication between the
user and kernel space.
Add support to use Netlink, and updated README.md for
instructions to enable this support.