Re: [PATCH RFC v2] netfilter: add connlabel conntrack extension

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Florian,

On Mon, Nov 12, 2012 at 01:30:53PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > int nfct_label_set(struct nf_conntrack *ct, const char *label);
> > > 
> > > 	sets the label 'label' on the ct object.
> > > 
> > > void nfct_label_unset(struct nf_conntrack *ct, const char *label);
> > > 
> > > 	opposite, label is cleared if it was set.
> > 
> > Can you use the existing nfct_attr_set/unset API for this?
> 
> Don't see how, nfct_attr_unset clears the entire attribute.
> I admit that it would be nice if one could re-use existing api...
> 
> Right now nfct_attr_set_l(.. ATTR_CONNLABELS..) can be used to assign
> the bit-vector directly (and _get can be used to retrieve the
> label bit-vector).

You could use a nfct_bitmask object and methods to modify it:

struct nfct_bitmask *nfct_bitmask_alloc(void)
nfct_bitmask_set_bit(struct nfct_bitmask *, uint16_t bit)
nfct_bitmask_test_bit(struct nfct_bitmask *, uint16_t bit)
nfct_bitmask_unset_bit(struct nfct_bitmask *, uint16_t bit)

Then you can attach the bitmask to the object:

nfct_set_attr(ct, ATTR_CONNLABELS, bitmask);

The setter would do the special handling internally by attaching the
bitmask label to the ct object.

> > > open question is how the library should do the mapping, i.e.
> > > should it hard-code a path to the mapping file (currently
> > > its /etc/xtables/connlabel.conf in my iptables-patch).
> > 
> > Add a new parameter to nfct_label_open to indicate where the file that
> > contains the mapping is.
> 
> I think that we should avoid it; else this might become a
> portability nightmare where applications end up
> trying half a zillion names (/etc/xtables, /usr/etc, /usr/local...)

We can provide some constant to recommend a path. My only concern is
that the hardcoded path may not exist in some filesystem, thus,
forcing to recompile the library.

Anyway, I think the main user for this will be the conntrack utility
and any netfilter utilities.

One more question below:

> > > void *nfct_label_open(void);
> > > void nfct_label_close(void *);
> > > int nfct_label_iterate(void *fp, char *buf, size_t buflen);
> > > 
> > > so you could do
> > > void *h = nfct_label_open();
> > > int bit;
> > > while ((bit = nfct_label_iterate(h, bufm sizeof(buf))) > 0)
> > > 	printf("bit %d: %s\n", bit, buf);
> > > 
> > > ?
> > 
> > That seems fine to me.
> 
> Thanks.  I now have the following public API:
> 
> /**
>  * nfct_label_set_bit - set a bit in the label bitvector
>  * \param ct pointer to a valid conntrack object
>  * \param bit bit to set
>  * returns 0 if the bit is now set
>  */
> int nfct_label_set_bit(struct nf_conntrack *ct, uint16_t bit)
> {
>         return __label_set_bit(ct, bit);
> }
> 
> /**
>  * nfct_label_unset_bit - clear label bit on a conntrack object
>  * \param ct pointer to a valid conntrack object
>  * \param bit bit to clear
>  */
> void nfct_label_unset(struct nf_conntrack *ct, uint16_t bit)
> {
>         __label_unset_bit(ct, bit);
> }
> 
> /**
>  * nfct_label_test_bit - determine wheter bit is set
>  * \param ct pointer to a valid conntrack object
>  * \param bit bit to query
>  *
>  * returns 0 on sucess and -1 if bit is not set.
>  */
> int nfct_label_test_bit(struct nf_conntrack *ct, uint16_t bit)
> {
>         return __label_test_bit(ct, bit);
> }
> 
> /**
>  * nfct_label_get_max - get highest bit set in label vector
>  * \param ct pointer to a valid conntrack object
>  * returns highest bit set, or -1 if no bits are set.
>  */
> int nfct_label_get_max(struct nf_conntrack *ct)
> {
>         return __label_get_max(ct);
> }
> 
> /**
>  * nfct_label_open
>  * \param open the default bit-name mapping file for reading.
>  *
>  * returns a FILE handle to be passed to nfct_label_iterate.
>  */
> void *nfct_label_open(void);
> 
> /**
>  * nfct_label_iterate
>  * \param handle handle obtained from nfct_label_open
>  * \param buf buffer where name will be stored
>  * \param len size of the buffer
>  *
>  * returns the bit the name is mapped to, or -1 when no more
>  * labels are defined.  The handle should be closed via
>  * fclose() once it is of no more interest.
>  */
> int nfct_label_iterate(void *handle, char *buf, size_t len);

One question, what is void *handle? Is it hidding a FILE * or a cache
containing the bitmask?
 
You can define some:

struct nfct_label_handle;

in libnetfilter_conntrack.h, and keep the layout private by defining
it in some internal header.

I'd go for the cache-based approach, ie. store a lookup array in
memory extracted from the file.

> /**
>  * @}
>  */
> 
> Since nfct_label_open() now returns FILE* userspace can provide other
> mapping files as well [ nfct_label_open just looks at the default location ].
> 
> I've killed the functions that took a "const char *labelname" as
> argument; it's error prone to fiddle with hidden file-descriptor
> in the library (not thread-safe...).
> 
> We could later add another library that provides more sophisticated
> stuff, such as a label<->bit cache.
> 
> The patch that i currently have (untested, so I won't send it yet...)
> adds about 300 LOC, i would like to avoid bloating libnetfilter_conntrack
> more than that.

Examples files usually help to see how the API looks in action, if you
can attach one in the next round, in would be great.

Regards.