netfilter: REJECT: separate reusable code

netfilter: REJECT: separate reusable code

[Patrick McHardy]

Just noticed commit cc70d069 (netfilter: REJECT: separate reusable code).
That doesn't look like a good idea to me at all.

First of all, it introduces static non-inline functions into a header file,
which is obviously wrong. But more importantly, it adds a symbol dependency
of the reject module on IPv6. We've tried hard to get rid of all these in
x_tables, lets please not re-add them in nftables.

I think we should instead use AF-specific modules for things like that.
We share basically no code except the boiler plate. I'd suggest to add
an AF-specific expression type lookup mechanism that takes precedence
over generic types.

Any other opinions? I wouldn't mind taking care of this myself.

Re: netfilter: REJECT: separate reusable code

[Pablo Neira Ayuso]

On Wed, Feb 05, 2014 at 10:25:48AM +0000, Patrick McHardy wrote:
> Just noticed commit cc70d069 (netfilter: REJECT: separate reusable code).
> That doesn't look like a good idea to me at all.
> 
> First of all, it introduces static non-inline functions into a header file,
> which is obviously wrong. But more importantly, it adds a symbol dependency
> of the reject module on IPv6. We've tried hard to get rid of all these in
> x_tables, lets please not re-add them in nftables.

Well, I think this is working at this moment, we can improve it of
course.

> I think we should instead use AF-specific modules for things like that.
> We share basically no code except the boiler plate. I'd suggest to add
> an AF-specific expression type lookup mechanism that takes precedence
> over generic types.

Yes, this looks like the way to go to me.

How do you plan to handle this with the inet table? We don't have
family context there.

Re: netfilter: REJECT: separate reusable code

[Patrick McHardy]

On Wed, Feb 05, 2014 at 11:42:47AM +0100, Pablo Neira Ayuso wrote:
> On Wed, Feb 05, 2014 at 10:25:48AM +0000, Patrick McHardy wrote:
> > Just noticed commit cc70d069 (netfilter: REJECT: separate reusable code).
> > That doesn't look like a good idea to me at all.
> > 
> > First of all, it introduces static non-inline functions into a header file,
> > which is obviously wrong. But more importantly, it adds a symbol dependency
> > of the reject module on IPv6. We've tried hard to get rid of all these in
> > x_tables, lets please not re-add them in nftables.
> 
> Well, I think this is working at this moment, we can improve it of
> course.

Sure. I'd just prefer to have it done right from the beginning instead
of having to fix it up afterwards if we already know of these problems.
It just takes more time this way.

I wonder if we could add something that would break compilation for
anything in net/netfilter depending on the ipv6 module. It keeps
happening again and again.

> > I think we should instead use AF-specific modules for things like that.
> > We share basically no code except the boiler plate. I'd suggest to add
> > an AF-specific expression type lookup mechanism that takes precedence
> > over generic types.
> 
> Yes, this looks like the way to go to me.
> 
> How do you plan to handle this with the inet table? We don't have
> family context there.

Hmm good question indeed. We do have it at runtime, but this would
obviously also mean we'd also have to dispatch at runtime.

I guess an NFPROTO_INET specific reject module that dispatches to
the IPv4 and IPv6 versions is the only possibility unless we want
to add restrictions (which I don't).

Re: netfilter: REJECT: separate reusable code

[Pablo Neira Ayuso]

On Wed, Feb 05, 2014 at 10:53:48AM +0000, Patrick McHardy wrote:
> On Wed, Feb 05, 2014 at 11:42:47AM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Feb 05, 2014 at 10:25:48AM +0000, Patrick McHardy wrote:
> > > Just noticed commit cc70d069 (netfilter: REJECT: separate reusable code).
> > > That doesn't look like a good idea to me at all.
> > > 
> > > First of all, it introduces static non-inline functions into a header file,
> > > which is obviously wrong. But more importantly, it adds a symbol dependency
> > > of the reject module on IPv6. We've tried hard to get rid of all these in
> > > x_tables, lets please not re-add them in nftables.
> > 
> > Well, I think this is working at this moment, we can improve it of
> > course.
> 
> Sure. I'd just prefer to have it done right from the beginning instead
> of having to fix it up afterwards if we already know of these problems.
> It just takes more time this way.
> 
> I wonder if we could add something that would break compilation for
> anything in net/netfilter depending on the ipv6 module. It keeps
> happening again and again.

We are using this trick thing in Kconfig:

depends on (IPV6 || IPV6=n)

and similar thing in NFT_REJECT

depends on NF_TABLES_IPV6 || !NF_TABLES_IPV6

it's not nice, but it seems to work to avoid buildbot reports.

> > > I think we should instead use AF-specific modules for things like that.
> > > We share basically no code except the boiler plate. I'd suggest to add
> > > an AF-specific expression type lookup mechanism that takes precedence
> > > over generic types.
> > 
> > Yes, this looks like the way to go to me.
> > 
> > How do you plan to handle this with the inet table? We don't have
> > family context there.
> 
> Hmm good question indeed. We do have it at runtime, but this would
> obviously also mean we'd also have to dispatch at runtime.
> 
> I guess an NFPROTO_INET specific reject module that dispatches to
> the IPv4 and IPv6 versions is the only possibility unless we want
> to add restrictions (which I don't).

I think that, once the infrastructure to provide expressions per
family in place, a specific reject for inet is a good idea. It can
reply depending on the packet family that it sees at _eval(...). I
don't have any better idea on how to handle this case.

Re: netfilter: REJECT: separate reusable code

[Arturo Borrero Gonzalez]

On 5 February 2014 12:17, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>
>> I guess an NFPROTO_INET specific reject module that dispatches to
>> the IPv4 and IPv6 versions is the only possibility unless we want
>> to add restrictions (which I don't).
>
> I think that, once the infrastructure to provide expressions per
> family in place, a specific reject for inet is a good idea. It can
> reply depending on the packet family that it sees at _eval(...). I
> don't have any better idea on how to handle this case.

Just wondering if this idea could be reused to allow nft_payload to
fetch ip src/dst in a family independent way, so we can have dual
stacked rules of this kind:

nft add rule inet filter input ip daddr www.example.com accept

or maybe:

nft add rule inet filter input ip daddr { 1.1.1.1 : accept , ::1 : accept }

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: netfilter: REJECT: separate reusable code

[Arturo Borrero Gonzalez]

On 5 February 2014 12:49, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
>
> Just wondering if this idea could be reused to allow nft_payload to
> fetch ip src/dst in a family independent way, so we can have dual
> stacked rules of this kind:

I meant dependant,.
-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: netfilter: REJECT: separate reusable code

[Patrick McHardy]

On Wed, Feb 05, 2014 at 12:17:13PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Feb 05, 2014 at 10:53:48AM +0000, Patrick McHardy wrote:
> > On Wed, Feb 05, 2014 at 11:42:47AM +0100, Pablo Neira Ayuso wrote:
> > > On Wed, Feb 05, 2014 at 10:25:48AM +0000, Patrick McHardy wrote:
> > > > Just noticed commit cc70d069 (netfilter: REJECT: separate reusable code).
> > > > That doesn't look like a good idea to me at all.
> > > > 
> > > > First of all, it introduces static non-inline functions into a header file,
> > > > which is obviously wrong. But more importantly, it adds a symbol dependency
> > > > of the reject module on IPv6. We've tried hard to get rid of all these in
> > > > x_tables, lets please not re-add them in nftables.
> > > 
> > > Well, I think this is working at this moment, we can improve it of
> > > course.
> > 
> > Sure. I'd just prefer to have it done right from the beginning instead
> > of having to fix it up afterwards if we already know of these problems.
> > It just takes more time this way.
> > 
> > I wonder if we could add something that would break compilation for
> > anything in net/netfilter depending on the ipv6 module. It keeps
> > happening again and again.
> 
> We are using this trick thing in Kconfig:
> 
> depends on (IPV6 || IPV6=n)
> 
> and similar thing in NFT_REJECT
> 
> depends on NF_TABLES_IPV6 || !NF_TABLES_IPV6
> 
> it's not nice, but it seems to work to avoid buildbot reports.

That's not what I meant. I *want* to break compilation if anything in
net/netfilter depends on a symbol of the IPV6 module. Its wrong, with
the only exception being stuff for NFPROTO_INET.

> > > > I think we should instead use AF-specific modules for things like that.
> > > > We share basically no code except the boiler plate. I'd suggest to add
> > > > an AF-specific expression type lookup mechanism that takes precedence
> > > > over generic types.
> > > 
> > > Yes, this looks like the way to go to me.
> > > 
> > > How do you plan to handle this with the inet table? We don't have
> > > family context there.
> > 
> > Hmm good question indeed. We do have it at runtime, but this would
> > obviously also mean we'd also have to dispatch at runtime.
> > 
> > I guess an NFPROTO_INET specific reject module that dispatches to
> > the IPv4 and IPv6 versions is the only possibility unless we want
> > to add restrictions (which I don't).
> 
> I think that, once the infrastructure to provide expressions per
> family in place, a specific reject for inet is a good idea. It can
> reply depending on the packet family that it sees at _eval(...). I
> don't have any better idea on how to handle this case.

Yes, that seems fine, its in fact pretty much what we're doing right
now.

Re: netfilter: REJECT: separate reusable code

[Patrick McHardy]

On Wed, Feb 05, 2014 at 12:49:01PM +0100, Arturo Borrero Gonzalez wrote:
> On 5 February 2014 12:17, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >>
> >> I guess an NFPROTO_INET specific reject module that dispatches to
> >> the IPv4 and IPv6 versions is the only possibility unless we want
> >> to add restrictions (which I don't).
> >
> > I think that, once the infrastructure to provide expressions per
> > family in place, a specific reject for inet is a good idea. It can
> > reply depending on the packet family that it sees at _eval(...). I
> > don't have any better idea on how to handle this case.
> 
> Just wondering if this idea could be reused to allow nft_payload to
> fetch ip src/dst in a family independent way, so we can have dual
> stacked rules of this kind:
> 
> nft add rule inet filter input ip daddr www.example.com accept
> 
> or maybe:
> 
> nft add rule inet filter input ip daddr { 1.1.1.1 : accept , ::1 : accept }

Nope, ip daddr implies meta nfproto == NFPROTO_IPV4. Also we have a length
in the payload instruction. We might be able to do something by mapping
IPv4 addresses into IPv6 address space.