Mekhanik evgenii/ctx size reducing

[EvgeniiMekhanik]

No description provided.

[EvgeniiMekhanik]

Totally this patch change bpftool btf dump file bpf/BUILD/xdp.o format raw | grep XfwGlobalCtx
from [259] STRUCT 'XfwGlobalCtx' size=112 vlen=14 to [259] STRUCT 'XfwGlobalCtx' size=80 vlen=12
We save 32 bytes! But we add extra check before getting appropriate pointers, because of verifier restrictions.

[consuelo2210]

I just started to review code. In debug mode I have:

libbpf: prog 'xfw_tc': failed to load: -13
libbpf: failed to load object '/opt/tempesta/lib/bpf/tc.o'

Don't fix it until the end of the review, please.

[consuelo2210]

Overall, I like the direction of these changes. The refactoring makes the parsing code cleaner and more structured, and I think moving to offsets instead of pointers is the right approach. From the verifier's perspective, it's easier to derive a new pointer from an offset and perform a fresh bounds check than to track an existing pointer. I also believe this approach will make it easier to increase the program's complexity in the future(code complexity is a part of evolution) and to work with specific headers only when needed, as you did in tcp_syncookies.h.

My main concern is that the current code looks like it's only halfway through the refactoring. It already contains parts of the new design, but the overall code structure still follows the old approach. Since we're already making these changes, I think it's much easier to complete the refactoring now than to leave it for later. Otherwise, it will most likely be forgotten.

So, please do an additional self-review on top of my comments and try to identify any remaining inconsistencies or illogical code paths introduced by these changes.

Also, please add const to the pointer parameters you've started passing to functions wherever appropriate.

One more thing: please compile the code in DEBUG mode as well. At the moment, the DEBUG build of tc.o cannot be loaded onto the interface.

[consuelo2210]

I am not sure that we need this field. Do we really have cases now when eth_off is greater than 0?

[consuelo2210]

If we remove eth_off entirely, we may not need any parameters for parse_ethhdr() besides hdr_cur. In fact, we might not need to advance hdr_cur at all and could instead calculate the required pointer from the base address whenever needed(by the way, may we setup ip_off in parse_ethhdr?).

I also noticed that all offsets stored in the context are uint8_t, but they're computed like uint16_t:

*eth_off = (uint16_t)((void *)eth - data_bgn);

typedef struct XfwGlobalCtx {
	XfwHdrCursor	hdr_cur;
	XfwIp		ilog_addr;
	XfwFilterCfg	*cfg;
	XfwPerCpuStats	*g_stats;
	XfwMd		*ctx;
	uint64_t	ts_jiff;
	uint32_t	pkt_sz;
	uint16_t	ipver;
	**uint8_t		ip_off;
	uint8_t		l4_off;**
	uint8_t		l4_proto;
	**uint8_t		eth_off;**
} XfwGlobalCtx;

I took a quick look at the supported IPv6 extension headers:

• Hop-by-Hop Options (0)
• Routing (43)
• Authentication Header (AH, 51)
• Destination Options (60)
• Mobility Header (135)

ip_off and eth_off should always fit into uint8_t, but l4_off may not if an IPv6 packet contains a long chain of extension headers. In that case, uint16_t would be a safer type for l4_off.

[consuelo2210]

1. It seems a bit strange to pass data_bgn and l4_off here. Both values can be calculated outside this function, so we don't need parse_tcphdr() to populate them. We could even initialize l4_off at the L3 parsing stage.
   The same applies to the other parse_* functions from parsing_helpers.h.

2. We also calculate struct tcphdr *th twice now: once inside parse_tcphdr(), and then immediately afterwards with additional checks:

if (unlikely(parse_tcphdr(&ctx->hdr_cur,
                          XFW_CTX_DATA_BGN(ctx->ctx),
                          &ctx->l4_off) <= 0))
    return XFW_MAKE_CTX_DROP(ctx, XFW_TCP_BADHDR_INGRESS);

struct tcphdr *th =
    XFW_GET_PKT_PTR(ctx, ctx->l4_off, struct tcphdr,
                    XFW_MAKE_CTX_DROP(ctx, XFW_TCP_BADHDR_INGRESS));

[consuelo2210]

@EvgeniiMekhanik Could you please take a look at this?

68a6595

The tradeoff is a larger ctx, since it stores pointers instead of offsets, but it should be faster. Overall, after the cleanup, only four boundary checks were added.