Skip to content

nat46-module: Add support for Netlink sockets #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
srsmiraato wants to merge 1 commit into
base: master
Choose a base branch
from
Open

nat46-module: Add support for Netlink sockets #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@ This is an OpenWRT feed with a Linux kernel module implementing flexible NAT46.
Compiling
=========

The module by default uses procfs for communication between the user and kernel space.
To use Netlink sockets instead, add the following to nat46/nat46/modules/Makefile
when compiling:
```
EXTRA_CFLAGS += -DPROTO_NETLINK
Copy link
Copy Markdown
Owner

@ayourtch ayourtch May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The define should be a bit more specific for NAT46 module and more self-descriptive. NAT46_USE_NETLINK_CONFIG or something like that.

Copy link
Copy Markdown
Author

@srsmiraato srsmiraato May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback. I'll keep your suggestion in mind as I update my patch.

```

With Barrier Breaker (trunk), add the following line to *feeds.conf.default*:
```
src-git nat46 https://github.com/ayourtch/nat46.git
Expand Down
104 changes: 83 additions & 21 deletions nat46/modules/nat46-module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@
#include <linux/proc_fs.h> // for the proc filesystem
#include <linux/seq_file.h> // for sequence files

#ifdef PROTO_NETLINK
/* Netlink IPC */
#include <linux/netlink.h>
#include <linux/skbuff.h>
#include <net/sock.h>
#endif

#include <net/ip.h>
#include <net/tcp.h>
#include <net/udp.h>
Expand All @@ -50,6 +57,10 @@
#define NAT46_VERSION __DATE__ " " __TIME__
#endif

#ifdef PROTO_NETLINK
#define NETLINK_USER 31
Copy link
Copy Markdown
Owner

@ayourtch ayourtch May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Copy Markdown
Author

@srsmiraato srsmiraato May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I'll keep your suggestion in mind as I update my patch.

#endif /* PROTO_NETLINK */

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Andrew Yourtchenko <ayourtch@gmail.com>");
MODULE_DESCRIPTION("NAT46 stateless translation");
Expand All @@ -61,6 +72,9 @@ MODULE_PARM_DESC(debug, "debugging messages level (default=1)");
static struct proc_dir_entry *nat46_proc_entry;
static struct proc_dir_entry *nat46_proc_parent;

#ifdef PROTO_NETLINK
struct sock *nl_sk = NULL;
#endif /* PROTO_NETLINK */

static int nat46_proc_show(struct seq_file *m, void *v)
{
Expand All @@ -86,13 +100,58 @@ static char *get_devname(char **ptail)
return devname;
}

static void
nat46_proc_cmd(char *cmd)
{
char *arg = NULL;
char *devname = NULL;

while(NULL != (arg = get_next_arg (&cmd))) {
if(0 == strcmp(arg, "add")) {
devname = get_devname(&cmd);
printk(KERN_INFO "nat46: adding device (%s)\n", devname);
nat46_create(devname);
}
else if(0 == strcmp(arg, "del")) {
devname = get_devname(&cmd);
printk(KERN_INFO "nat46: deleting device (%s)\n", devname);
nat46_destroy(devname);
}
else if(0 == strcmp(arg, "config")) {
devname = get_devname(&cmd);
printk(KERN_INFO "nat46: configure device (%s) with '%s'\n", devname, cmd);
nat46_configure(devname, cmd);
}
else if(0 == strcmp(arg, "insert")) {
devname = get_devname(&cmd);
printk(KERN_INFO "nat46: insert new rule into device (%s) with '%s'\n",
devname, cmd);
nat46_insert(devname, cmd);
}
}
}

#ifdef PROTO_NETLINK
static void
nat46_nl_recv_msg(struct sk_buff *skb_in)
{
struct nlmsghdr *nl_hdr;
int pid;
char *cmd = NULL;

nl_hdr = (struct nlmsghdr *) skb_in->data;
pid = nl_hdr->nlmsg_pid;
cmd = (char *) nlmsg_data (nl_hdr);

nat46_proc_cmd(cmd);
Copy link
Copy Markdown
Owner

@ayourtch ayourtch May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Author

@srsmiraato srsmiraato May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
#endif /* PROTO_NETLINK */

static ssize_t nat46_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char *buf = NULL;
char *tail = NULL;
char *devname = NULL;
char *arg_name = NULL;

buf = kmalloc(sizeof(char) * (count + 1), GFP_KERNEL);
if (!buf)
Expand All @@ -108,25 +167,7 @@ static ssize_t nat46_proc_write(struct file *file, const char __user *buffer,
buf[count-1] = '\0';
}

while (NULL != (arg_name = get_next_arg(&tail))) {
if (0 == strcmp(arg_name, "add")) {
devname = get_devname(&tail);
printk(KERN_INFO "nat46: adding device (%s)\n", devname);
nat46_create(devname);
} else if (0 == strcmp(arg_name, "del")) {
devname = get_devname(&tail);
printk(KERN_INFO "nat46: deleting device (%s)\n", devname);
nat46_destroy(devname);
} else if (0 == strcmp(arg_name, "config")) {
devname = get_devname(&tail);
printk(KERN_INFO "nat46: configure device (%s) with '%s'\n", devname, tail);
nat46_configure(devname, tail);
} else if (0 == strcmp(arg_name, "insert")) {
devname = get_devname(&tail);
printk(KERN_INFO "nat46: insert new rule into device (%s) with '%s'\n", devname, tail);
nat46_insert(devname, tail);
}
}
nat46_proc_cmd(tail);

kfree(buf);
return count;
Expand Down Expand Up @@ -157,6 +198,21 @@ int create_nat46_proc_entry(void) {

static int __init nat46_init(void)
{
#ifdef PROTO_NETLINK
struct netlink_kernel_cfg nl_cfg = {
.input = nat46_nl_recv_msg
};
printk("nat46: module (version %s) loaded.\n", NAT46_VERSION);
Copy link
Copy Markdown
Owner

@ayourtch ayourtch May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two places printing the exact same content is confusing. If anything, this place should print something about procfs support not present, please use netlink.

Copy link
Copy Markdown
Author

@srsmiraato srsmiraato May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Copy Markdown
Owner

@ayourtch ayourtch May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be more descriptive. Mention that it is a netlink control socket.

Copy link
Copy Markdown
Author

@srsmiraato srsmiraato May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I'll keep your suggestion in mind as I update my patch.

return -1;
}

return 0;
#else
int ret = 0;

printk("nat46: module (version %s) loaded.\n", NAT46_VERSION);
Expand All @@ -168,17 +224,23 @@ static int __init nat46_init(void)

error:
return ret;
#endif /* PROTO_NETLINK */
}

static void __exit nat46_exit(void)
{
#ifdef PROTO_NETLINK
netlink_kernel_release(nl_sk);
nat46_destroy_all();
#else
nat46_destroy_all();
if (nat46_proc_parent) {
if (nat46_proc_entry) {
remove_proc_entry(NAT46_CONTROL_PROC_NAME, nat46_proc_parent);
}
remove_proc_entry(NAT46_PROC_NAME, init_net.proc_net);
}
#endif /* PROTO_NETLINK */
printk("nat46: module unloaded.\n");
}

Expand Down