Re: [PATCH] Set mark to 0 from libnetfilter_conntrack

Eric Leblond wrote:
> Hi,
>
> Damien Boucard from INL has discovered a bug in libnetfilter_conntrack :
> Mark can not be set to 0.
>
> After looking at the code I've found that we only change the mark if it
> is not set to 0 :
>     if (ct->mark != 0)
>         nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_MARK, &mark,
>          sizeof(u_int32_t));
>
> What's the cleanest way to solve this. I don't see any mean to correct
> this except adding an IPS_CHANGE_MARK flag.
>
> Proposed patch is attached to the mail.
>
> BR,
>
>
> ------------------------------------------------------------------------
>
> Index: include/libnetfilter_conntrack/libnetfilter_conntrack.h
> ===================================================================
> --- include/libnetfilter_conntrack/libnetfilter_conntrack.h    (revision 6689)
> +++ include/libnetfilter_conntrack/libnetfilter_conntrack.h    (working copy)
> @@ -196,6 +196,10 @@
>     IPS_FIXED_TIMEOUT_BIT = 10,
>     IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT),
>
> + /* Connectio must change MARK */
> +    IPS_CHANGE_MARK_BIT = 11,
> +    IPS_CHANGE_MARK = (1 << IPS_FIXED_CHANGE_MARK),
> +
> };
>
> enum {
> Index: src/libnetfilter_conntrack.c
> ===================================================================
> --- src/libnetfilter_conntrack.c    (revision 6689)
> +++ src/libnetfilter_conntrack.c    (working copy)
> @@ -976,7 +976,7 @@
>     nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_TIMEOUT, &timeout,
>          sizeof(u_int32_t));
>     
> -    if (ct->mark != 0)
> +    if (ct->status & IPS_CHANGE_MARK)
>         nfnl_addattr_l(&req->nlh, sizeof(buf), CTA_MARK, &mark,
>              sizeof(u_int32_t));
>

I don't see anything setting this bit and it shouldn't be a conntrack
bit if it doesn't exist in the kernel (and we certainly don't want this
in the kernel). The idea is still the right one, I think the library
should take care of adding a CTA_MARK attribute without any user bitmask
fiddling by including it if the value differs from the mark contained in
the received conntrack. I think Pablo's new API will allow this.