2016-09-13 17:19 GMT+08:00 Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>:
> Hi Liping,
>
> A bit more comments on top of Florian's suggestion to use one single
> _SREG.
>
> On Sun, Sep 11, 2016 at 10:05:28PM +0800, Liping Zhang wrote:
>> diff --git a/net/netfilter/nft_queue.c b/net/netfilter/nft_queue.c
>> static void nft_queue_eval(const struct nft_expr *expr,
>> @@ -32,17 +34,30 @@ static void nft_queue_eval(const struct nft_expr *expr,
>> const struct nft_pktinfo *pkt)
>> {
>> struct nft_queue *priv = nft_expr_priv(expr);
>> - u32 queue = priv->queuenum;
>> + u32 queue, queues_total, queue_end;
>> u32 ret;
>>
>> - if (priv->queues_total > 1) {
>> + if (priv->sreg_from) {
>> + queue = (u16)regs->data[priv->sreg_from];
>> + queue_end = (u16)regs->data[priv->sreg_to];
>> +
>> + if (queue_end > queue)
>> + queues_total = queue_end - queue + 1;
>> + else
>> + queues_total = 1;
>> + } else {
>> + queue = priv->queuenum;
>> + queues_total = priv->queues_total;
>> + }
>
> I suggest you use .select_ops to use a specific _eval function if
> _SREG is set. This would disentangle this code a bit.
>
>>
>> + /* nftables has no idea whether the kernel supports _SREG_FROM or not,
>> + * so for compatibility reason, it may specify the _SREG_FROM and
>> + * _QUEUE_NUM attributes at the same time. We prefer to use _SREG_FROM,
>> + * it is more flexible and has less restriction, for example, queue
>> + * range 0-65535 is ok when use _SREG_FROM and _SREG_TO.
>> + */
>
> No need for this large comment, look.
>
> From userspace, we have to modify the nft parser to take an expression
> as 'num'. Then, from the netlink_linearize path, we can check the
> expression type:
>
> * If it's a value expression, we can simply use NFTA_QUEUE_NUM.
> * If it's a range expression, we have to set up NFTA_QUEUE_NUM and
> NFTA_QUEUE_TOTAL.
> * If it's a mapping, then we only use _SREG.
>
> Old kernels will bail out if the mapping is used, as this is not
> supported, which is what we expect. This should be fine by now, until
> I finish the patchset to provide a VM description that userspace can
> use to generate bytecode depending on the features available in the
> kernel.
>
>>
>> @@ -97,9 +142,21 @@ static int nft_queue_dump(struct sk_buff *skb, const struct nft_expr *expr)
>> {
>> const struct nft_queue *priv = nft_expr_priv(expr);
>>
>> - if (nla_put_be16(skb, NFTA_QUEUE_NUM, htons(priv->queuenum)) ||
>> - nla_put_be16(skb, NFTA_QUEUE_TOTAL, htons(priv->queues_total)) ||
>> - nla_put_be16(skb, NFTA_QUEUE_FLAGS, htons(priv->flags)))
>> + if (priv->sreg_from) {
>> + if (nft_dump_register(skb, NFTA_QUEUE_SREG_FROM,
>> + priv->sreg_from))
>> + goto nla_put_failure;
>> + if (nft_dump_register(skb, NFTA_QUEUE_SREG_TO,
>> + priv->sreg_to))
>> + goto nla_put_failure;
>> + } else {
>> + if (nla_put_be16(skb, NFTA_QUEUE_NUM, htons(priv->queuenum)) ||
>> + nla_put_be16(skb, NFTA_QUEUE_TOTAL,
>> + htons(priv->queues_total)))
>> + goto nla_put_failure;
>> + }
>
> Looking at this, the code will look better if we should use
> .select_ops indeed.
>
> Thanks for resolving existing nft_queue limitations!
I will re-spin my patch based on your and Florian's suggestions.
Thanks Pablo.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
