(re-send): Convert libnetfilter_queue to not need libnfnetlink]

(re-send): Convert libnetfilter_queue to not need libnfnetlink]

[Duncan Roe]

Hi Pablo,

On Mon, Sep 11, 2023 at 09:51:07AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Sep 11, 2023 at 03:54:25PM +1000, Duncan Roe wrote:
[SNIP]
> > libnetfilter_queue effectively supports 2 ABIs, the older being based on
> > libnfnetlink and the newer on libmnl.
>
> Yes, there are two APIs, same thing occurs in other existing
> libnetfilter_* libraries, each of these APIs are based on libnfnetlink
> and libmnl respectively.
>
[SNIP]
>
> libnfnetlink will go away sooner or later. We are steadily replacing
> all client of this library for netfilter.org projects. Telling that
> this is not deprecated without providing a compatible "old API" for
> libmnl adds more confusion to this subject.
>
> If you want to explore providing a patch that makes the
> libnfnetlink-based API work over libmnl, then go for it.

OK I went for it. But I posted the resultant patchset as a reply to an
earlier email.

The Patchwork series is
https://patchwork.ozlabs.org/project/netfilter-devel/list/?series=399143
("Convert nfq_open() to use libmnl").

The series is "code only" - I kept back the documentation changes for
spearate review. These documentation changes present the "old API" as
merely an alternative to the mnl API: both use libmnl.

Do you think you might find time to look at it before too long? I know you
are very busy but I would appreciate some feedback.

Cheers ... Duncan.

Re: (re-send): Convert libnetfilter_queue to not need libnfnetlink]

[Pablo Neira Ayuso]

Hi Duncan,

On Fri, Mar 29, 2024 at 08:31:13AM +1100, Duncan Roe wrote:
> Hi Pablo,
> 
> On Mon, Sep 11, 2023 at 09:51:07AM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Sep 11, 2023 at 03:54:25PM +1000, Duncan Roe wrote:
> [SNIP]
> > > libnetfilter_queue effectively supports 2 ABIs, the older being based on
> > > libnfnetlink and the newer on libmnl.
> >
> > Yes, there are two APIs, same thing occurs in other existing
> > libnetfilter_* libraries, each of these APIs are based on libnfnetlink
> > and libmnl respectively.
> >
> [SNIP]
> >
> > libnfnetlink will go away sooner or later. We are steadily replacing
> > all client of this library for netfilter.org projects. Telling that
> > this is not deprecated without providing a compatible "old API" for
> > libmnl adds more confusion to this subject.
> >
> > If you want to explore providing a patch that makes the
> > libnfnetlink-based API work over libmnl, then go for it.
> 
> OK I went for it. But I posted the resultant patchset as a reply to an
> earlier email.
> 
> The Patchwork series is
> https://patchwork.ozlabs.org/project/netfilter-devel/list/?series=399143
> ("Convert nfq_open() to use libmnl").
> 
> The series is "code only" - I kept back the documentation changes for
> spearate review. These documentation changes present the "old API" as
> merely an alternative to the mnl API: both use libmnl.

Thanks for explaining.

> Do you think you might find time to look at it before too long? I know you
> are very busy but I would appreciate some feedback.

This update is large it comes with its own risks and I see chances
that existing applications might break with this "transparent"
approach (where user is not aware that libnfnetlink is not used
anymore).

So far main complains with the new API is that it is too low level
(some users do not want to know about netlink details). The old API is
popular because it provides an easy way for users to receive packets
from the nfnetlink subsystem without dealing with netlink details.

My suggestion is to extend the new API with more functions to make it
ressemble more like the old API. Then, document how to migrate from
the old API to the new API, such documentation would be good to
include a list of items with things that have changed between old and
new APIs.

Would you consider feasible to follow up in this direction? If so,
probably you can make new API proposal that can be discussed.

I hope this does not feel discouraging to you, I think all this work
that you have done will be useful in this new approach and likely you
can recover ideas from this patchset.

Thanks for your patience.

Re: (re-send): Convert libnetfilter_queue to not need libnfnetlink]

[Duncan Roe]

Hi Pablo,

On Fri, Mar 29, 2024 at 11:01:56PM +0100, Pablo Neira Ayuso wrote:
> Hi Duncan,
>
> On Fri, Mar 29, 2024 at 08:31:13AM +1100, Duncan Roe wrote:
> > Hi Pablo,
> >
> > On Mon, Sep 11, 2023 at 09:51:07AM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Sep 11, 2023 at 03:54:25PM +1000, Duncan Roe wrote:
> > [SNIP]
> > > > libnetfilter_queue effectively supports 2 ABIs, the older being based on
> > > > libnfnetlink and the newer on libmnl.
> > >
> > > Yes, there are two APIs, same thing occurs in other existing
> > > libnetfilter_* libraries, each of these APIs are based on libnfnetlink
> > > and libmnl respectively.
> > >
> > [SNIP]
> > >
> > > libnfnetlink will go away sooner or later. We are steadily replacing
> > > all client of this library for netfilter.org projects. Telling that
> > > this is not deprecated without providing a compatible "old API" for
> > > libmnl adds more confusion to this subject.
> > >
> > > If you want to explore providing a patch that makes the
> > > libnfnetlink-based API work over libmnl, then go for it.
> >
> > OK I went for it. But I posted the resultant patchset as a reply to an
> > earlier email.
> >
> > The Patchwork series is
> > https://patchwork.ozlabs.org/project/netfilter-devel/list/?series=399143
> > ("Convert nfq_open() to use libmnl").
> >
> > The series is "code only" - I kept back the documentation changes for
> > spearate review. These documentation changes present the "old API" as
> > merely an alternative to the mnl API: both use libmnl.
>
> Thanks for explaining.
>
> > Do you think you might find time to look at it before too long? I know you
> > are very busy but I would appreciate some feedback.
>
> This update is large

Yes, there are 32 patches. Unlike in my first attempt at this conversion,
you can apply these patches one at a time without breaking anything.

P1 is the critical patch: it sets up the private structures used by
libnfnetlink (using copied code) and libnetfilter_queue (by calling
mnl_socket_open()) so that calls to either library work. You would want to
carefully review the copied code that sets up the libnfnetlink private
structures. Once you are satisfied that part is sound, patches up to p10
convert all other nfq_* functions.

p11 - p32 do pretty much what their titles say. Mostly they implement and
document the nlif_* functions from libnfnetlink.

N.B. nfq_close() will leak memory until you apply p3. It might be best to
treat p1 - p3 as a single patch. I can re-issue the series that way if you
would prefer.

> it comes with its own risks and I see chances
> that existing applications might break with this "transparent"
> approach (where user is not aware that libnfnetlink is not used
> anymore).

Hmm. You do say above that "libnfnetlink will go away sooner or later". For
it to go away, you will need something like this patchset.
>
> So far main complains with the new API is that it is too low level
> (some users do not want to know about netlink details). The old API is
> popular because it provides an easy way for users to receive packets
> from the nfnetlink subsystem without dealing with netlink details.
>
> My suggestion is to extend the new API with more functions to make it
> ressemble more like the old API. Then, document how to migrate from
> the old API to the new API, such documentation would be good to
> include a list of items with things that have changed between old and
> new APIs.
>
> Would you consider feasible to follow up in this direction? If so,
> probably you can make new API proposal that can be discussed.
>
> I hope this does not feel discouraging to you, I think all this work
> that you have done will be useful in this new approach and likely you
> can recover ideas from this patchset.

Will address these points in a separate email.
>
> Thanks for your patience.
>
Cheers ... Duncan.

Re: (re-send): Convert libnetfilter_queue to not need libnfnetlink

[Duncan Roe]

Hi Pablo,

On Fri, Mar 29, 2024 at 11:01:56PM +0100, Pablo Neira Ayuso wrote:
> Hi Duncan,
>
[SNIP]
>
> ... Then, document how to migrate from
> the old API to the new API, such documentation would be good to
> include a list of items with things that have changed between old and
> new APIs.

Waste of time I think. People are not going to migrate - what's in it for them?

*We* have to do the migration, because *we* then get to ditch libnfnetlink.

Over the years you have mentioned a desire to do that. Now it's done.
>
> Would you consider feasible to follow up in this direction? If so,
> probably you can make new API proposal that can be discussed.

Actually yes. It would make things easier for new users of libnetfilter_queue.
Will detail in another email.
>
> I hope this does not feel discouraging to you, I think all this work
> that you have done will be useful in this new approach and likely you
> can recover ideas from this patchset.
>
> Thanks for your patience.
>
Cheers ... Duncan.

Re: (re-send): Convert libnetfilter_queue to not need libnfnetlink]

[Duncan Roe]

Hi Pablo,

On Fri, Mar 29, 2024 at 11:01:56PM +0100, Pablo Neira Ayuso wrote:
> Hi Duncan,
>
[SNIP]
>
> ... Then, document how to migrate from
> the old API to the new API, such documentation would be good to
> include a list of items with things that have changed between old and
> new APIs.

Waste of time I think. People are not going to migrate - what's in it for them?

*We* have to do the migration, because *we* then get to ditch libnfnetlink.

Over the years you have mentioned a desire to do that. Now it's done.
>
> Would you consider feasible to follow up in this direction? If so,
> probably you can make new API proposal that can be discussed.

Actually yes. It would make things easier for new users of libnetfilter_queue.
Will detail in another email.
>
> I hope this does not feel discouraging to you, I think all this work
> that you have done will be useful in this new approach and likely you
> can recover ideas from this patchset.
>
> Thanks for your patience.
>
Cheers ... Duncan.

PS Sorry if you got this twice - D

Re: (re-send): Convert libnetfilter_queue to not need libnfnetlink]

[Duncan Roe]

Hi Pablo,

On Fri, Mar 29, 2024 at 11:01:56PM +0100, Pablo Neira Ayuso wrote:
> Hi Duncan,
>
[SNIP]
>
> This update is large ...

Yes it's too large. It's actually 2 separate patchsets run together:
 1. Eliminate libnfnetlink calls & headers from libnetfilter_queue (11 patches)
 2. Add all the nlif_* functions from libnfnetlink (21 patches)

> ... I see chances
> that existing applications might break with this "transparent"
> approach ...

Did you have anything specific in mind?

After I gdb-stepped through patched and unpatched code, all I could find was
nfq_open_nfnl() is missing its EBUSY check - easily fixed. Oh and internal
buffers are dimensioned MNL_SOCKET_BUFFER_SIZE (min of architecture page size
and 8192) where they used to be NFNL_BUFFSIZE (always 8192).

Patches 01/32-03/32 assure that existing old-API programs can continue to use
direct libnfnetlink calls:

Patch 01/32: Convert nfq_open() adds code taken from libnfnetlink to create a
fully populated struct nfnl_handle. This enables other functions to continue to
use libnfnetlink calls.

Patch 02/32: Convert nfq_open_nfnl() is far larger than it needs to be. It
converts the code added in patch 01/32 into a static function (which is how I
missed the EBUSY check) - I'll put the static function in patch 01/32 next time.
Other than that patch 02/32 sets up a struct mnl_socket from the data in the
struct nfnl_handle.

Patch 03/32: Convert nfq_close() calls mnl_socket_close() and adds code taken
from libnfnetlink to dispose of the struct nfnl_handle.

How about if I submit a v2 with only patches 01 - 11? That's enough so a
libnetfilter_queue build no longer needs libnfnetlink.

Cheers ... Duncan.