[PATCH] netfilter: nftables: fix documentation for dup statement

[PATCH] netfilter: nftables: fix documentation for dup statement

[Quentin Armitage]

The dup statement requires an address, and the device is optional,
not the other way round.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 doc/statements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/statements.txt b/doc/statements.txt
index 9155f286..835db087 100644
--- a/doc/statements.txt
+++ b/doc/statements.txt
@@ -648,7 +648,7 @@ The dup statement is used to duplicate a packet and send the
copy to a different
 destination.
 
 [verse]
-*dup to* 'device'
+*dup to* 'address'
 *dup to* 'address' *device* 'device'
 
 .Dup statement values
-- 
2.25.4

Re: [PATCH] netfilter: nftables: fix documentation for dup statement

[Phil Sutter]

Hi,

On Thu, Aug 27, 2020 at 04:42:00PM +0100, Quentin Armitage wrote:
> The dup statement requires an address, and the device is optional,
> not the other way round.
> 
> Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
> ---
>  doc/statements.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/doc/statements.txt b/doc/statements.txt
> index 9155f286..835db087 100644
> --- a/doc/statements.txt
> +++ b/doc/statements.txt
> @@ -648,7 +648,7 @@ The dup statement is used to duplicate a packet and send the
> copy to a different
>  destination.
>  
>  [verse]
> -*dup to* 'device'
> +*dup to* 'address'
>  *dup to* 'address' *device* 'device'
>  
>  .Dup statement values

The examples are wrong, too. I wonder if this is really just a mistake
and all three examples given (including the "advanced" usage using a
map) are just wrong or if 'dup' actually was meant to support
duplicating to a device in mirror port fashion.

Florian, you wrote the docs. What's your take here?

Thanks, Phil

Re: [PATCH] netfilter: nftables: fix documentation for dup statement

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Hi,
> 
> On Thu, Aug 27, 2020 at 04:42:00PM +0100, Quentin Armitage wrote:
> > The dup statement requires an address, and the device is optional,
> > not the other way round.
> > 
> > Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
> > ---
> >  doc/statements.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/doc/statements.txt b/doc/statements.txt
> > index 9155f286..835db087 100644
> > --- a/doc/statements.txt
> > +++ b/doc/statements.txt
> > @@ -648,7 +648,7 @@ The dup statement is used to duplicate a packet and send the
> > copy to a different
> >  destination.
> >  
> >  [verse]
> > -*dup to* 'device'
> > +*dup to* 'address'
> >  *dup to* 'address' *device* 'device'
> >  
> >  .Dup statement values
> 
> The examples are wrong, too. I wonder if this is really just a mistake
> and all three examples given (including the "advanced" usage using a
> map) are just wrong or if 'dup' actually was meant to support
> duplicating to a device in mirror port fashion.

Right, 'dup to eth0' can be used in the netdev ingress hook.

For dup from ipv4/ipv6 families the address is needed.

Re: [PATCH] netfilter: nftables: fix documentation for dup statement

[Quentin Armitage]

On Thu, 2020-08-27 at 19:40 +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Hi,
> > 
> > On Thu, Aug 27, 2020 at 04:42:00PM +0100, Quentin Armitage wrote:
> > > The dup statement requires an address, and the device is optional,
> > > not the other way round.
> > > 
> > > Signed-off-by: Quentin Armitage <
> > > quentin@armitage.org.uk
> > > >
> > > ---
> > >  doc/statements.txt | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/doc/statements.txt b/doc/statements.txt
> > > index 9155f286..835db087 100644
> > > --- a/doc/statements.txt
> > > +++ b/doc/statements.txt
> > > @@ -648,7 +648,7 @@ The dup statement is used to duplicate a packet and
> > > send the
> > > copy to a different
> > >  destination.
> > >  
> > >  [verse]
> > > -*dup to* 'device'
> > > +*dup to* 'address'
> > >  *dup to* 'address' *device* 'device'
> > >  
> > >  .Dup statement values
> > 
> > The examples are wrong, too. I wonder if this is really just a mistake
> > and all three examples given (including the "advanced" usage using a
> > map) are just wrong or if 'dup' actually was meant to support
> > duplicating to a device in mirror port fashion.
> 
> Right, 'dup to eth0' can be used in the netdev ingress hook.
> 
> For dup from ipv4/ipv6 families the address is needed.

So it seems the valid options are:
*dup to* 'device'			# netdev ingress hook only
*dup to* 'address'  			# ipv4/ipv6 only
*dup to* 'address' *device* 'device'	# ipv4/ipv6 only

From a user perspective being able to specify "dup to 'device'" is something
that is useful to be able to specify. I am now using:
  dup to ip[6] daddr device 'device'
but it seems to me that having to specify "to ip[6] daddr" is unnecessary.

So far as I can see, it would be quite straightforward to allow "dup to
'device'" to be specified and for nft to handle it with an implied "to ip[6]
addr". I am happy to produce a patch to do this if it would be helpful.

I am also happy to submit a revised patch for statements.txt if that would be
useful.

Quentin Armitage

Re: [PATCH] netfilter: nftables: fix documentation for dup statement

[Phil Sutter]

Hi Quentin,

On Thu, Aug 27, 2020 at 07:59:19PM +0100, Quentin Armitage wrote:
> On Thu, 2020-08-27 at 19:40 +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > Hi,
> > > 
> > > On Thu, Aug 27, 2020 at 04:42:00PM +0100, Quentin Armitage wrote:
> > > > The dup statement requires an address, and the device is optional,
> > > > not the other way round.
> > > > 
> > > > Signed-off-by: Quentin Armitage <
> > > > quentin@armitage.org.uk
> > > > >
> > > > ---
> > > >  doc/statements.txt | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/doc/statements.txt b/doc/statements.txt
> > > > index 9155f286..835db087 100644
> > > > --- a/doc/statements.txt
> > > > +++ b/doc/statements.txt
> > > > @@ -648,7 +648,7 @@ The dup statement is used to duplicate a packet and
> > > > send the
> > > > copy to a different
> > > >  destination.
> > > >  
> > > >  [verse]
> > > > -*dup to* 'device'
> > > > +*dup to* 'address'
> > > >  *dup to* 'address' *device* 'device'
> > > >  
> > > >  .Dup statement values
> > > 
> > > The examples are wrong, too. I wonder if this is really just a mistake
> > > and all three examples given (including the "advanced" usage using a
> > > map) are just wrong or if 'dup' actually was meant to support
> > > duplicating to a device in mirror port fashion.
> > 
> > Right, 'dup to eth0' can be used in the netdev ingress hook.
> > 
> > For dup from ipv4/ipv6 families the address is needed.
> 
> So it seems the valid options are:
> *dup to* 'device'			# netdev ingress hook only
> *dup to* 'address'  			# ipv4/ipv6 only
> *dup to* 'address' *device* 'device'	# ipv4/ipv6 only
> 
> From a user perspective being able to specify "dup to 'device'" is something
> that is useful to be able to specify. I am now using:
>   dup to ip[6] daddr device 'device'
> but it seems to me that having to specify "to ip[6] daddr" is unnecessary.

Oh, and that works? From reading nf_dup_ipv4.c, the kernel seems to
perform a route lookup for the packet's daddr on given iface. Did you
add an onlink route or something to make sure that succeeds?

Cheers, Phil

Re: [PATCH] netfilter: nftables: fix documentation for dup statement

[Quentin Armitage]

Hi Phil,

On Mon, 2020-08-31 at 18:49 +0200, Phil Sutter wrote:
> Hi Quentin,
> 
> On Thu, Aug 27, 2020 at 07:59:19PM +0100, Quentin Armitage wrote:
> > On Thu, 2020-08-27 at 19:40 +0200, Florian Westphal wrote:
> > > Phil Sutter <
> > > phil@nwl.cc
> > > > wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Aug 27, 2020 at 04:42:00PM +0100, Quentin Armitage wrote:
> > > > > The dup statement requires an address, and the device is optional,
> > > > > not the other way round.
> > > > > 
> > > > > Signed-off-by: Quentin Armitage <
> > > > > quentin@armitage.org.uk
> > > > > 
> > > > > 
> > > > > ---
> > > > >  doc/statements.txt | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/doc/statements.txt b/doc/statements.txt
> > > > > index 9155f286..835db087 100644
> > > > > --- a/doc/statements.txt
> > > > > +++ b/doc/statements.txt
> > > > > @@ -648,7 +648,7 @@ The dup statement is used to duplicate a packet
> > > > > and
> > > > > send the
> > > > > copy to a different
> > > > >  destination.
> > > > >  
> > > > >  [verse]
> > > > > -*dup to* 'device'
> > > > > +*dup to* 'address'
> > > > >  *dup to* 'address' *device* 'device'
> > > > >  
> > > > >  .Dup statement values
> > > > 
> > > > The examples are wrong, too. I wonder if this is really just a mistake
> > > > and all three examples given (including the "advanced" usage using a
> > > > map) are just wrong or if 'dup' actually was meant to support
> > > > duplicating to a device in mirror port fashion.
> > > 
> > > Right, 'dup to eth0' can be used in the netdev ingress hook.
> > > 
> > > For dup from ipv4/ipv6 families the address is needed.
> > 
> > So it seems the valid options are:
> > *dup to* 'device'			# netdev ingress hook only
> > *dup to* 'address'  			# ipv4/ipv6 only
> > *dup to* 'address' *device* 'device'	# ipv4/ipv6 only
> > 
> > From a user perspective being able to specify "dup to 'device'" is something
> > that is useful to be able to specify. I am now using:
> >   dup to ip[6] daddr device 'device'
> > but it seems to me that having to specify "to ip[6] daddr" is unnecessary.
> 
> Oh, and that works? From reading nf_dup_ipv4.c, the kernel seems to
> perform a route lookup for the packet's daddr on given iface. Did you
> add an onlink route or something to make sure that succeeds?
> 
> Cheers, Phil

It is working for me, both with IPv4 and IPv6, and I suspect the reason is that
I am using this for multicast packets. In particular, I have a macvlan and I want to join multicast groups on the macvlan interface but I want the IGMP/MLD join group messages to be sent with the MAC address of the "parent" interface of the macvlan, and not the mac address of the macvlan itself.

The rules I am using are:
 map vmac_map {
	type iface_index : iface_index
	elements = { "macvlan0" : "eth0" }
 }

 ip protocol igmp dup to ip daddr device oif map @vmac_map drop
and
 icmpv6 type mld2-listener-report dup to ip6 daddr device oif map @vmac_map drop

With many thanks for your help,

Quentin

Re: [PATCH] netfilter: nftables: fix documentation for dup statement

[Pablo Neira Ayuso]

Hi Quentin,

Thanks for your patch:

On Thu, Aug 27, 2020 at 04:42:00PM +0100, Quentin Armitage wrote:
> 
> The dup statement requires an address, and the device is optional,
> not the other way round.

table netdev x {
        chain y {
                type filter hook ingress device "eth0" priority filter; policy accept;
                ip protocol udp dup to "eth1"
        }
}

I think probably it should be good to clarify that:

- dup to 'device'
- fwd to 'device'

only work from the netdev family.

Thanks.

> Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
> ---
>  doc/statements.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/doc/statements.txt b/doc/statements.txt
> index 9155f286..835db087 100644
> --- a/doc/statements.txt
> +++ b/doc/statements.txt
> @@ -648,7 +648,7 @@ The dup statement is used to duplicate a packet and send the
> copy to a different
>  destination.
>  
>  [verse]
> -*dup to* 'device'
> +*dup to* 'address'
>  *dup to* 'address' *device* 'device'
>  
>  .Dup statement values
> -- 
> 2.25.4
> 
>