Comment 2 for bug 1554072

Revision history for this message
Christof Mroz (christof-mroz) wrote :

Does the attached patch to hipfw/rewrite.c happen to solve or improve the issue? It seems like the ipq compatibility struct declares "payload" as a pointer, while "payload" was in fact a zero-length array in the original struct. This requires some additional pointer bending.

Has midauth really been tested since merging the nf_queue branch in revision 6420, beyond running the unit tests? Does reverting to 6419 help? I can't test it myself at the moment, but perhaps later this week.

Regarding your temporary solution, I don't recommend leaving the "assume_ipq_buffer_sufficient" flag on permanently at the moment for two reasons:

1. We only have at most HIP_MAX_DATA - x bytes of payload available, where "x" denotes the size of the nf_queue per-packet metadata, but the "splice_param" function currently assumes at least HIP_MAX_DATA bytes. This may be easy to fix by inferring x via pointer arithmetic and passing down the actual remaining space to splice_param via the context structure.

2. It's not clear to me whether or not recv() on the nf_queue socket can in read multiple packets or not. The NFQ docs imply that only a single packet get copied at once, but reading the source code of libnfnl suggests otherwise. Indeed, the callback-driven interface makes it effortless (to the library user) to process multiple packets at once, so why wouldn't they make use of this feature? In any case, multiple packets are a problem because enlarging e.g. the first packet would overwrite the beginning of the second one, if they share the same buffer. So moving memory in one form or another is necessary unless we are manipulating the very last packet in the buffer and the remaining space suffices.

I'd keep the flag around for debug and benchmarking purposes though (e.g. for measuring the impact of copying on effective throughput). Perhaps the purpose of this flag should be more clearly documented?