Re: [PATCH 3/3 nft] src: osf: import nfnl_osf.c to load osf fingerprints

[Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx>]

On 08/11/2018 12:03 PM, Pablo Neira Ayuso wrote:
> > +#endif /* _NF_OSF_H */
> > diff --git a/include/nfnl_osf.h b/include/nfnl_osf.h
> > new file mode 100644
> > index 0000000..d9287e9
> > --- /dev/null
> > +++ b/include/nfnl_osf.h
> > @@ -0,0 +1,6 @@
> > +#ifndef _NFNL_OSF_H
> > +#define _NFNL_OSF_H
> > +
> > +int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, int del);
> > +
> > +#endif	/* _NFNL_OSF_H */
> > diff --git a/include/osf.h b/include/osf.h
> > index 715b04e..0a35b07 100644
> > --- a/include/osf.h
> > +++ b/include/osf.h
> > @@ -1,6 +1,8 @@
> >   #ifndef NFTABLES_OSF_H
> >   #define NFTABLES_OSF_H
> >   
> > +bool osf_init;
>
> I think you can probably place osf_init in struct netlink_ctx?

If we place osf_init in struct netlink_ctx we will need to modify 
osf_expr_alloc() and I am not sure if we can get access to netlink_ctx 
from netlink_parse_osf() in netlink_delinearize.c. Also we will need 
access to netlink_ctx from parser_bison.y.

So I propose to add osf_init in nfnl_osf.h in order to have only one 
extra include in rule.c. Thanks.

> >   struct expr *osf_expr_alloc(const struct location *loc);
> >   
> >   #endif /* NFTABLES_OSF_H */
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index ed3640e..e569029 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -57,6 +57,7 @@ libnftables_la_SOURCES =			\
> >   		services.c			\
> >   		mergesort.c			\
> >   		osf.c				\
> > +		nfnl_osf.c			\
> >   		tcpopt.c			\
> >   		socket.c			\
> >   		libnftables.c
> > diff --git a/src/nfnl_osf.c b/src/nfnl_osf.c
> > new file mode 100644
> > index 0000000..07bf682
> > --- /dev/null
> > +++ b/src/nfnl_osf.c
> > @@ -0,0 +1,449 @@
> > +/*
> > + * Copyright (c) 2005 Evgeniy Polyakov <johnpol@xxxxxxxxxx>
> > + *
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> > + */
> > +
> > +#include <sys/time.h>
> > +
> > +#include <ctype.h>
> > +#include <errno.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <time.h>
> > +
> > +#include <netinet/ip.h>
> > +#include <netinet/tcp.h>
> > +
> > +#include <linux/unistd.h>
> > +
> > +#include <libmnl/libmnl.h>
> > +
> > +#include <linux/netfilter/nfnetlink.h>
> > +#include <linux/netfilter/nfnetlink_osf.h>
> > +#include <mnl.h>
> > +#include <nfnl_osf.h>
> > +
> > +#define OPTDEL			','
> > +#define OSFPDEL 		':'
> > +#define MAXOPTSTRLEN		128
> > +
> > +static struct nf_osf_opt IANA_opts[] = {
> > +	{ .kind = 0, .length = 1,},
> > +	{ .kind=1, .length=1,},
> > +	{ .kind=2, .length=4,},
> > +	{ .kind=3, .length=3,},
> > +	{ .kind=4, .length=2,},
> > +	{ .kind=5, .length=1,},		/* SACK length is not defined */
> > +	{ .kind=6, .length=6,},
> > +	{ .kind=7, .length=6,},
> > +	{ .kind=8, .length=10,},
> > +	{ .kind=9, .length=2,},
> > +	{ .kind=10, .length=3,},
> > +	{ .kind=11, .length=1,},		/* CC: Suppose 1 */
> > +	{ .kind=12, .length=1,},		/* the same */
> > +	{ .kind=13, .length=1,},		/* and here too */
> > +	{ .kind=14, .length=3,},
> > +	{ .kind=15, .length=1,},		/* TCP Alternate Checksum Data. Length is not defined */
> > +	{ .kind=16, .length=1,},
> > +	{ .kind=17, .length=1,},
> > +	{ .kind=18, .length=3,},
> > +	{ .kind=19, .length=18,},
> > +	{ .kind=20, .length=1,},
> > +	{ .kind=21, .length=1,},
> > +	{ .kind=22, .length=1,},
> > +	{ .kind=23, .length=1,},
> > +	{ .kind=24, .length=1,},
> > +	{ .kind=25, .length=1,},
> > +	{ .kind=26, .length=1,},
> > +};
> > +
> > +static void uloga(const char *f, struct netlink_ctx *ctx, ...)
> > +{
> > +	if (!(ctx->debug_mask & NFT_DEBUG_NETLINK))
> > +		return;
> > +
> > +	nft_print(ctx->octx, "%s", f);
> > +}
>
> I think you can use uloga() all the time, so you can remove ulog()
> function.

I agree. Changes done.

> > +static void ulog(const char *f, struct netlink_ctx *ctx, ...)
> > +{
> > +	char str[64];
> > +	struct tm tm;
> > +	struct timeval tv;
> > +
> > +	gettimeofday(&tv, NULL);
> > +	localtime_r((time_t *)&tv.tv_sec, &tm);
> > +	strftime(str, sizeof(str), "%F %R:%S", &tm);
> > +
> > +	if (!(ctx->debug_mask & NFT_DEBUG_NETLINK))
> > +		return;
> > +
> > +	nft_print(ctx->octx, "%s.%lu %ld %s", str, tv.tv_usec,
> > +		  syscall(__NR_gettid), f);
> > +}
> > +
> > +#define ulog_err(f, ctx, a...) uloga(f ": %s [%d].\n", ctx, ##a, strerror(errno), errno)
>
> And this macro too.
>
> Other than that, this looks good to me, thanks.