Re: [RFC PATCH] netfilter: nf_tables: extend payload to support writing data

[Patrick McHardy <kaber@trash.net>]

On Mon, Feb 17, 2014 at 07:43:38PM +0100, Nikolay Aleksandrov wrote:
> On 02/17/2014 07:37 PM, Patrick McHardy wrote:
> >>  
> >> -static void nft_payload_eval(const struct nft_expr *expr,
> >> -			     struct nft_data data[NFT_REG_MAX + 1],
> >> -			     const struct nft_pktinfo *pkt)
> >> +static int nft_payload_make_offset(const struct nft_pktinfo *pkt,
> >> +				   const struct nft_payload *payload)
> > 
> > We're using priv everywhere. Please keep this consistent. Also please
> > make sure (in the entire patch) that arguments are aligned with the
> > opening (.
> > 
> Okay, I'll use the priv.
> They're adjusted, apply the patch and see for yourself, or do you mean some
> other adjustment ?
> Both arguments are aligned from what I can see.

I see. Probably misrepresented by mutt.

> >> +static void nft_payload_set_eval(const struct nft_expr *expr,
> >> +				 struct nft_data data[NFT_REG_MAX + 1],
> >> +				 const struct nft_pktinfo *pkt)
> >> +{
> >> +	const struct nft_payload *priv = nft_expr_priv(expr);
> >> +	struct nft_data *source = &data[priv->sreg];
> >> +	int offset = nft_payload_make_offset(pkt, priv);
> >> +
> >> +	if (offset == -1)
> >> +		goto err;
> >> +	if (!skb_make_writable(pkt->skb, offset + priv->len))
> >> +		goto err;
> >> +	memcpy(pkt->skb->data + offset, source, priv->len);
> > 
> > We need to take care of checksumming. Shouldn't be too hard to get *most*
> > cases right using the existing checksum helpers.
> > 
> Yes, but would you like to do that in here or have a separate op which
> is configurable i.e. you can set what to calculate and where to put it,
> or would you prefer to make it automatic based on what we're changing here ?

Something here is a lot cheaper since we can use incremental checksumming.
That won't be possible somewhere else, additionally we'd have to check
the checksum before recalculating it to make sure we don't fix up bad
packets. So yes, I think it should be done here.