On Wed, Jan 17, 2018 at 07:31:33PM +0100, Thierry Du Tre wrote:
> Op 16/01/2018 om 15:32 schreef Pablo Neira Ayuso:
> > Hi Thierry,
> >
> > On Mon, Jan 15, 2018 at 01:56:09PM +0100, Thierry Du Tre wrote:
> >> Hi Pablo,
> >>
> >> I prepared this third version to get aligned about the way forward for the extension for struct nf_nat_range.
> >>
> >> Renaming the old definition as you suggested indeed results in a much smaller patch for netfilter kernel part.
> >> However, doing it like this also means that userspace code will require changes to cope with the new value for sizeof(struct nf_nat_range).
> >>
> >> i.e. iptables-1.6.1 :
> >>
> >> ./extensions/libip6t_SNAT.c:306: .userspacesize = XT_ALIGN(sizeof(struct nf_nat_range)),
> >> ./extensions/libip6t_DNAT.c:290: .userspacesize = XT_ALIGN(sizeof(struct nf_nat_range)),
> >> ./extensions/libip6t_NETMAP.c:89: .userspacesize = XT_ALIGN(sizeof(struct nf_nat_range)),
> >> ./extensions/libip6t_MASQUERADE.c:159: .userspacesize = XT_ALIGN(sizeof(struct nf_nat_range)),
> >> ./extensions/libip6t_REDIRECT.c:158: .userspacesize = XT_ALIGN(sizeof(struct nf_nat_range)),
> >>
> >> As far as I understand, all these xt target modules will have to increment their revision which makes them incompatible with current kernel versions.
> >> The other option is to replace all occurences of nf_nat_range with nf_nat_range1 in these userspace libraries.
> >> That would solve iptables but possible other applications might also be impacted ?
> >>
> >> Somehow this doesn't seem right to me, so I might have misinterpreted your earlier response.
> >
> > I guess you need to add new revisions for the userspace code too,
> > right? Am I missing anything?
>
> I attached an example change for libipt6t_SNAT in iptables. This
> illustrates how renaming current nf_nat_range should be handled in
> userspace code. If we would just increment the revision number in
> snat_tg_reg, then a new kernel with support for revision 2 would be
> required for creating SNAT rules. So for compatibility with
> existing kernels, we need to keep current SNAT revision in but adapt
> to the renamed nf_nat_range datastruct.
Renaming in userspace is fine. If that's the concern, there is no
problem if this helps us keep the kernel patch small.
> ---
> extensions/libip6t_SNAT.c | 20 ++++++++++----------
> include/linux/netfilter/nf_nat.h | 19 +++++++++++++++++--
> 2 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/extensions/libip6t_SNAT.c b/extensions/libip6t_SNAT.c
> index 7d74b3d..3eb0319 100644
> --- a/extensions/libip6t_SNAT.c
> +++ b/extensions/libip6t_SNAT.c
> @@ -47,7 +47,7 @@ static const struct xt_option_entry SNAT_opts[] = {
>
> /* Ranges expected in network order. */
> static void
> -parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
> +parse_to(const char *orig_arg, int portok, nf_nat_range1 *range)
> {
> char *arg, *start, *end = NULL, *colon = NULL, *dash, *error;
> const struct in6_addr *ip;
> @@ -150,7 +150,7 @@ parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
> static void SNAT_parse(struct xt_option_call *cb)
> {
> const struct ip6t_entry *entry = cb->xt_entry;
> - struct nf_nat_range *range = cb->data;
> + nf_nat_range1 *range = cb->data;
> int portok;
>
> if (entry->ipv6.proto == IPPROTO_TCP ||
> @@ -182,7 +182,7 @@ static void SNAT_fcheck(struct xt_fcheck_call *cb)
> {
> static const unsigned int f = F_TO_SRC | F_RANDOM;
> static const unsigned int r = F_TO_SRC | F_RANDOM_FULLY;
> - struct nf_nat_range *range = cb->data;
> + nf_nat_range1 *range = cb->data;
>
> if ((cb->xflags & f) == f)
> range->flags |= NF_NAT_RANGE_PROTO_RANDOM;
> @@ -190,7 +190,7 @@ static void SNAT_fcheck(struct xt_fcheck_call *cb)
> range->flags |= NF_NAT_RANGE_PROTO_RANDOM_FULLY;
> }
>
> -static void print_range(const struct nf_nat_range *range)
> +static void print_range(const nf_nat_range1 *range)
> {
> if (range->flags & NF_NAT_RANGE_MAP_IPS) {
> if (range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)
> @@ -213,7 +213,7 @@ static void print_range(const struct nf_nat_range *range)
> static void SNAT_print(const void *ip, const struct xt_entry_target *target,
> int numeric)
> {
> - const struct nf_nat_range *range = (const void *)target->data;
> + const nf_nat_range1 *range = (const void *)target->data;
>
> printf(" to:");
> print_range(range);
> @@ -227,7 +227,7 @@ static void SNAT_print(const void *ip, const struct xt_entry_target *target,
>
> static void SNAT_save(const void *ip, const struct xt_entry_target *target)
> {
> - const struct nf_nat_range *range = (const void *)target->data;
> + const nf_nat_range1 *range = (const void *)target->data;
>
> printf(" --to-source ");
> print_range(range);
> @@ -239,7 +239,7 @@ static void SNAT_save(const void *ip, const struct xt_entry_target *target)
> printf(" --persistent");
> }
>
> -static void print_range_xlate(const struct nf_nat_range *range,
> +static void print_range_xlate(const nf_nat_range1 *range,
> struct xt_xlate *xl)
> {
> bool proto_specified = range->flags & NF_NAT_RANGE_PROTO_SPECIFIED;
> @@ -270,7 +270,7 @@ static void print_range_xlate(const struct nf_nat_range *range,
> static int SNAT_xlate(struct xt_xlate *xl,
> const struct xt_xlate_tg_params *params)
> {
> - const struct nf_nat_range *range = (const void *)params->target->data;
> + const nf_nat_range1 *range = (const void *)params->target->data;
> bool sep_need = false;
> const char *sep = " ";
>
> @@ -300,8 +300,8 @@ static struct xtables_target snat_tg_reg = {
> .version = XTABLES_VERSION,
> .family = NFPROTO_IPV6,
> .revision = 1,
> - .size = XT_ALIGN(sizeof(struct nf_nat_range)),
> - .userspacesize = XT_ALIGN(sizeof(struct nf_nat_range)),
> + .size = XT_ALIGN(sizeof(nf_nat_range1)),
> + .userspacesize = XT_ALIGN(sizeof(nf_nat_range1)),
> .help = SNAT_help,
> .x6_parse = SNAT_parse,
> .x6_fcheck = SNAT_fcheck,
> diff --git a/include/linux/netfilter/nf_nat.h b/include/linux/netfilter/nf_nat.h
> index 1ad3659..3d1c1e7 100644
> --- a/include/linux/netfilter/nf_nat.h
> +++ b/include/linux/netfilter/nf_nat.h
> @@ -9,10 +9,16 @@
> #define NF_NAT_RANGE_PROTO_RANDOM (1 << 2)
> #define NF_NAT_RANGE_PERSISTENT (1 << 3)
> #define NF_NAT_RANGE_PROTO_RANDOM_FULLY (1 << 4)
> +#define NF_NAT_RANGE_PROTO_OFFSET (1 << 5)
>
> #define NF_NAT_RANGE_PROTO_RANDOM_ALL \
> (NF_NAT_RANGE_PROTO_RANDOM | NF_NAT_RANGE_PROTO_RANDOM_FULLY)
>
> +#define NF_NAT_RANGE_MASK \
> + (NF_NAT_RANGE_MAP_IPS | NF_NAT_RANGE_PROTO_SPECIFIED | \
> + NF_NAT_RANGE_PROTO_RANDOM | NF_NAT_RANGE_PERSISTENT | \
> + NF_NAT_RANGE_PROTO_RANDOM_FULLY | NF_NAT_RANGE_PROTO_OFFSET)
> +
> struct nf_nat_ipv4_range {
> unsigned int flags;
> __be32 min_ip;
> @@ -26,12 +32,21 @@ struct nf_nat_ipv4_multi_range_compat {
> struct nf_nat_ipv4_range range[1];
> };
>
> -struct nf_nat_range {
> +typedef struct {
> unsigned int flags;
> union nf_inet_addr min_addr;
> union nf_inet_addr max_addr;
> union nf_conntrack_man_proto min_proto;
> union nf_conntrack_man_proto max_proto;
> -};
> +} nf_nat_range1;
In other existing code, eg. see extensions libxt_MARK.c, we use _v1
for this.
> +
> +typedef struct nf_nat_range {
> + unsigned int flags;
> + union nf_inet_addr min_addr;
> + union nf_inet_addr max_addr;
> + union nf_conntrack_man_proto min_proto;
> + union nf_conntrack_man_proto max_proto;
> + union nf_conntrack_man_proto base_proto;
> +} nf_nat_range2;
No typedef please as per kernel coding style.
--
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
