nftables batch abi broken ...

nftables batch abi broken ...

[Florian Westphal]

Hi.

batch handling in libnftnl uses this:
libnftnl/src/common.c:  nfg->res_id = NFNL_SUBSYS_NFTABLES;

It SHOULD be:
libnftnl/src/common.c:  nfg->res_id = htons(NFNL_SUBSYS_NFTABLES)

since res_id is a __be16.

The kernel contains the same error when decoding batch messages
which is why this works :-/

I found this problem when looking at sparse error reports on the kernel
where sparse complains about the following line in nfnetlink_rcv():

    nfnetlink_rcv_batch(skb, nlh, nfgenmsg->res_id);

and sparse complaint is correct, __be16 is treated as u16 without
conversion.

How to fix this?
If we want to maintain ABI on Little Endian only solution is to "fix"
it in kernel by annotating this with explicit cast to u16.

But it sucks since ->res_id is used via htons/ntohs in all other places
:-/

Any ideas?

Fix userspace and play 'guess todays endianess' in the kernel...?

Re: nftables batch abi broken ...

[Pablo Neira Ayuso]

On Thu, Aug 27, 2015 at 05:31:51PM +0200, Florian Westphal wrote:
> Hi.
> 
> batch handling in libnftnl uses this:
> libnftnl/src/common.c:  nfg->res_id = NFNL_SUBSYS_NFTABLES;
> 
> It SHOULD be:
> libnftnl/src/common.c:  nfg->res_id = htons(NFNL_SUBSYS_NFTABLES)
> 
> since res_id is a __be16.
> 
> The kernel contains the same error when decoding batch messages
> which is why this works :-/
> 
> I found this problem when looking at sparse error reports on the kernel
> where sparse complains about the following line in nfnetlink_rcv():
> 
>     nfnetlink_rcv_batch(skb, nlh, nfgenmsg->res_id);
> 
> and sparse complaint is correct, __be16 is treated as u16 without
> conversion.
> 
> How to fix this?
> If we want to maintain ABI on Little Endian only solution is to "fix"
> it in kernel by annotating this with explicit cast to u16.
> 
> But it sucks since ->res_id is used via htons/ntohs in all other places
> :-/
> 
> Any ideas?

We only have one subsystem that supports batching at this moment, ie.
nft, so we can add a workaround now, fix userspace and then drop the
workaround from the kernel at some point.