libnl icon indicating copy to clipboard operation
libnl copied to clipboard

Construction of nl_msg message with multiple headers

Open gszpetko opened this issue 3 years ago • 3 comments

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

gszpetko avatar May 05 '22 17:05 gszpetko

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

thom311 avatar May 05 '22 19:05 thom311

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.

gszpetko avatar May 09 '22 18:05 gszpetko

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.

thom311 avatar May 09 '22 19:05 thom311