Skip to content
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.

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

Construction of nl_msg message with multiple headers #313

Open
gszpetko opened this issue May 5, 2022 · 3 comments
Open

Construction of nl_msg message with multiple headers #313

gszpetko opened this issue May 5, 2022 · 3 comments

Comments

@gszpetko
Copy link

gszpetko commented May 5, 2022

Hi @thom311 ,

I'm looking for ability to send single message consisting of many NL headers in order to be able to configure kernel filtering changes in one transaction (to not disrupt the traffic). After checking libnl documentation and sources, it seems like API such as nfnlmsg_put() are suited for scenario of a single header within a message, i.e. succeeding calls to put are overwriting existing header rather than add after tail.

Can you recommend what would the be solution here? The closest W/A I found is to limit allocation size (i.e. via nlmsg_alloc_size) of a single message and encapsulate each header within message, then collect all of the segments in struct iovec, and execute send by nl_send_iovec.

For example, to construct a message with two NL headers:

// prepare messages with header, data, attributes, etc. 
// complete each message (i.e. seq_no, etc.)

struct iovec msg_iov[] = {
	{
		.iov_base = nlmsg_hdr(msg_1),
		.iov_len  = nlmsg_hdr(msg_1)->nlmsg_len,
	},
	{
		.iov_base = nlmsg_hdr(msg_2),
		.iov_len  = nlmsg_hdr(msg_2)->nlmsg_len,
	},
};

int bytes = nl_send_iovec(sk, msg_1, msg_iov, 2);
// proceeed with ACKs, etc.

It seems to work, but the main drawback is it involves multiple allocations.
Another W/A could be to have single instance of nl_msg and manipulate nm_nlh (along with size) to "fool" succeeding calls to nfnlmsg_put(), so it creates are header after the previous segment (including padding). But, this is risky (original pointer would have to be restored for deallocation) and is not possible with current API due to struct hiding.

Nevertheless, the cleanest solution might be for API to append segment. Do you consider it's something that could be added?

Thanks,
Grzegorz

@thom311
Copy link
Owner

thom311 commented May 5, 2022

hm. I am not familiar with this topic. And from your question, I can tell that you already know more than me.

A new API is certainly possible, if somebody provides a patch :)
For new API it's kinda relevant that there is a real life user, so we can see how it's used and that is turns out to be useful. What would also be great are unit tests. Though, so far our unit test coverage is very (very) bad, and I think I would not require unit tests for new code. So, that's not a requirement, but it would however be most appreciated.

As with help for your question. Maybe somebody else can answer on this issue. Or maybe you'd like to send an email to the mailing list? (though, in both cases I cannot guarantee that somebody will chime in). Good luck though!!

@gszpetko
Copy link
Author

gszpetko commented May 9, 2022

Thank you for quick reply.

I'm also new to this topic. To give a bit more insight, usage pattern is to apply filtering changes by nfnetlink batching. I believe this mechanism is specific to Netfilter subsystem-only.

Having an option to combine headers in general (i.e. not only for NF batching) may be also useful for non-functional, e.g. performance reasons. This seems in line with past mailing thread. Kernel likely limits maximum frame size (didn't check it yet), although default allocation message size (i.e. PAGE_SIZE) sounds reasonable to hold multiple headers.

IMHO, the cleanest solution may to be to modify semantics of nfnlmsg_put() and nlmsg_put(), so they append headers rather than overwrite by default. Such new sematic would be symmetrical with attributes API e.g. nla_put_u32(). Otherwise, the current behavior is a bit confusing meaning that put means actually two different things:

  • for attributes: to add attribute at end of message (including padding)
  • for headers: to overwrite header

Such change would break backward compatibility, although it's seems bit awkward to rely on header overwrite (but I may be wrong here and someone else considers it's a feature, not a bug). What do you think?

In either case, I agree with you that any of such enhancements deserve wider audience for discussion.

@thom311
Copy link
Owner

thom311 commented May 9, 2022

in general, changing behavior (and more importantly: breaking users) would be a problem.

I do think that it's unlikely that there are users who depend on this behavior, so we could change behavior. On the other hand, maybe we should just add another variant of the function, with the desired behavior. While that adds new API (and increase the complexity for the user to understand what to do), it seems the safe thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants