Bug 281082 - sys/netgraph/ng_ipfw.c: Using 32bit cookies breaks ipfw ngtee
Summary: sys/netgraph/ng_ipfw.c: Using 32bit cookies breaks ipfw ngtee
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Some People
Assignee: Eugene Grosbein
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2024-08-26 19:56 UTC by ruben
Modified: 2024-09-18 08:21 UTC (History)
1 user (show)

See Also:


Attachments
proposed fix (519 bytes, patch)
2024-09-12 17:44 UTC, Eugene Grosbein
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ruben 2024-08-26 19:56:42 UTC
The MFC’ed commit 20e1f207cc789a28783344614d6d1d1c639c5797 (https://cgit.freebsd.org/src/commit/?id=20e1f207cc789a28783344614d6d1d1c639c5797)
MFC’ed to 14.1 as dadf64c5586e5fa5e1018a3d8a02c9873b1121b8 and to 13.3 as 0b9242dea68c44dc630921d3802f3f80f4d84b48 breaks ipfw_netflow.

Reversing the patch restores functionality.

This might be due to 
* sys/netinet/ip_fw.h’s ipfw_insn->arg1 still sit at u_int16_t, perhaps truncating one or another so that it remains invisible for ng_ipfw and the rest of netgraph
* sbin/ipfw/ipfw2.c chkarg in case TOK_NGTEE poses a limit of IP_FW_TABLEARG (65535) on the ngtee parameter.

Tested on 13.3 with ipfw_netflow and a packetcapture on the receiving port / sudo flowctl netflow: show human

Reversing the patch seems less impacting than to figure out where arg1 handling needs to be adjusted for full 32bit operation.
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2024-08-27 05:47:50 UTC
Triage: over to committer of 20e1f207cc789a28783344614d6d1d1c639c5797.
Comment 2 Eugene Grosbein freebsd_committer freebsd_triage 2024-08-27 07:43:45 UTC
(In reply to ruben from comment #0)

Can you please supply a testcase? I mean, provide configuration to run and reproduce a problem.
Comment 3 ruben 2024-08-27 08:05:39 UTC
Test case, using 13.3-RELEASE-p5

cat << EOT | sudo tee -a /etc/rc.conf
firewall_type="open"
firewall_enable="YES"
ipfw_netflow_enable="YES"
EOT

$ sudo service ipfw start
$ sudo service ipfw_netflow start
$ sudo ngctl list -l
There are 4 total nodes:
  Name: ngctl83110      Type: socket          ID: 00000065   Num hooks: 0

  Name: ipfw            Type: ipfw            ID: 00000057   Num hooks: 1
  Local hook      Peer name       Peer type    Peer ID         Peer hook      
  ----------      ---------       ---------    -------         ---------      
  9995            netflow         netflow      0000005a        iface0         

  Name: netflow         Type: netflow         ID: 0000005a   Num hooks: 2
  Local hook      Peer name       Peer type    Peer ID         Peer hook      
  ----------      ---------       ---------    -------         ---------      
  export          netflow_export  ksocket      0000005b        inet/dgram/udp 
  iface0          ipfw            ipfw         00000057        9995           

  Name: netflow_export  Type: ksocket         ID: 0000005b   Num hooks: 1
  Local hook      Peer name       Peer type    Peer ID         Peer hook      
  ----------      ---------       ---------    -------         ---------      
  inet/dgram/udp  netflow         netflow      0000005a        export         

Wait a bit and check things with

$ sudo ipfw -at list 1000
01000    8530   3227551 Tue Aug 27 09:54:03 2024 ngtee 9995 ip from any to any

That shows packets should have been passed into netgraph
$ sudo flowctl netflow: show human
…
(No flows)

$ sudo ngctl msg netflow_export: getpeername
Rec'd response "getpeername" (6) from "[5b]:":
Args:   inet/127.0.0.1:9995

$ sudo tcpdump -n -l -vv -s0 -i lo0 port 9995
tcpdump: listening on lo0, link-type NULL (BSD loopback), capture size 262144 bytes
^C
0 packets captured
0 packets received by filter
0 packets dropped by kernel

——

Going to use the reverse parched ng_ipfw

$ sudo service ipfw_netflow stop 
$ sudo ngctl msg ipfw: shutdown         
$ sudo kldunload ng_ipfw                
$ sudo kldload /usr/obj/usr/src/amd64.amd64/sys/modules/netgraph/ipfw/ng_ipfw.ko

Make sure it was loaded

$ sudo kldstat -v | grep -A 4 ng_ipfw.ko
30    1 0xffffffff8315f000     2168 ng_ipfw.ko (/usr/obj/usr/src/amd64.amd64/sys/modules/netgraph/ipfw/ng_ipfw.ko)
        Contains modules:
                 Id Name
                547 ng_ipfw

Wait a bit for traffic to pass through and check with

$ sudo ipfw -at list 1000
01000    2265    773489 Tue Aug 27 10:03:58 2024 ngtee 9995 ip from any to any

And now we are seeing traffic again with flowctl

$ sudo flowctl netflow: show human                                              
SrcIf         SrcIPaddress    DstIf         DstIPaddress    Proto  SrcPort  DstPort     Pkts
(null)        192.168.1.20    bond0         172.30.0.13         1        0        0       10
bond0         192.168.1.2     bond0         239.255.255.250    17    50837     1900       30
bond0         192.168.1.95    bond0         255.255.255.255    17    59335     6667       20
bond0         192.168.1.3     bond0         239.255.255.250    17    43772     1900       10
bond0         192.168.1.21    bond0         224.0.0.18        112        0        0       74
bond0         192.168.1.87    bond0         239.255.255.250    17     1900     1900        6
bond0         192.168.1.35    bond0         224.0.0.251        17     5353     5353       11
bond0         192.168.1.13    bond0         239.255.255.250    17    33832     1900      520
Comment 4 Eugene Grosbein freebsd_committer freebsd_triage 2024-08-29 09:18:01 UTC
(In reply to ruben from comment #3)

Sorry for a delay.

I could not reproduce the problem using my 13.3-STABLE 2e95d0ed3 amd64 system updated this July with a kernel nearly identical to GENERIC. So, do you use GENERIC kernel or custom one?
Comment 5 Eugene Grosbein freebsd_committer freebsd_triage 2024-08-29 09:44:54 UTC
Do you really have the problem with default ipfw_netflow_hook=9995 or do you use hook value over 65536?
Comment 6 ruben 2024-08-29 10:42:55 UTC
Hi,

I think 65535 hooks should be plenty? So I’m happy with that. No need (for me) to have that 32 bits. That would probably require more fix ups in ipfw’s parameter handling. I also reversed the patch on 14.1 and that’s running fine

cd /usr/src && git -C /usr/git/freebsd-src.git show dadf64c5586e5fa5e1018a3d8a02c9873b1121b8 | sudo patch -R
cd /usr/src/sys/modules/netgraph/ipfw && sudo make && sudo make install
sudo sysrc kld_list+=/boot/modules/ng_ipfw.ko
Comment 7 Eugene Grosbein freebsd_committer freebsd_triage 2024-08-29 11:22:32 UTC
(In reply to ruben from comment #6)

It is not about total number of hooks/cookies. It is about possibility to encode 32 bits value into cookie without loosing any bits.
Comment 8 ruben 2024-09-06 14:03:42 UTC
(In reply to Eugene Grosbein from comment #4)
I’m not following stable, but stick with releases using freebsd-update. If this is not reproducible on -STABLE there could be other commits introduced after 13.3-RELEASE-p5 / 14.1-RELEASE-p3

I’ll try one of the 13.4-RC’s over the weekend.
Comment 9 Sergey Osipov 2024-09-12 14:11:57 UTC
(In reply to ruben from comment #3)
Hello, I did the same steps for 14.1-RELEASE-p4. Netflow doesn't work.
Comment 10 Eugene Grosbein freebsd_committer freebsd_triage 2024-09-12 15:28:19 UTC
(In reply to Sergey Osipov from comment #9)

Please show your setup.
Comment 11 Eugene Grosbein freebsd_committer freebsd_triage 2024-09-12 16:07:13 UTC
(In reply to Sergey Osipov from comment #9)

I've reproced the problem using 14.1-RELEASE/amd64. I'm investigating.
Comment 12 Eugene Grosbein freebsd_committer freebsd_triage 2024-09-12 17:44:13 UTC
Created attachment 253525 [details]
proposed fix

Please try this one-line patch to the module ng_ipfw.ko
Comment 13 ruben 2024-09-12 18:47:19 UTC
(In reply to Eugene Grosbein from comment #12)

the patch solves the issue for me on 
- 14.1-RELEASE-p4
- 13.3-RELEASE-p6

Thanks!
Comment 14 commit-hook freebsd_committer freebsd_triage 2024-09-12 19:16:43 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=becd0079c052cb87e7649b78733b99abae8861ee

commit becd0079c052cb87e7649b78733b99abae8861ee
Author:     Eugene Grosbein <eugen@FreeBSD.org>
AuthorDate: 2024-09-12 19:09:28 +0000
Commit:     Eugene Grosbein <eugen@FreeBSD.org>
CommitDate: 2024-09-12 19:09:28 +0000

    ng_ipfw(4): add missing change after previous commit

    The function ng_ipfw_input() used to enjoy implicit
    32->16 bits truncation of its second argument.
    Make it explicit to recover from the breakage.

    PR:             281082
    Reported by:    Ruben van Staveren <ruben@verweg.com>
    Tested by:      Ruben van Staveren <ruben@verweg.com>
    MFC after:      3 days
    Fixes:          20e1f207cc789a28783344614d6d1d1c639c5797

 sys/netgraph/ng_ipfw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 15 Sergey Osipov 2024-09-18 07:55:30 UTC
It works for me too (13.3-RELEASE-p5). Thank you.

Could you explain me, Is it going to include next patch? 
I haven't seen this patch in 13.4-RELEASE.

Thank's again.
Comment 16 Eugene Grosbein freebsd_committer freebsd_triage 2024-09-18 08:03:48 UTC
(In reply to Sergey Osipov from comment #15)

It was too late to merge the fix before 13.4-RELEASE. It is in 13.4-STABLE, though. And will be in 13.5-RELEASE.
Comment 17 Eugene Grosbein freebsd_committer freebsd_triage 2024-09-18 08:08:33 UTC
Oops, that was another late MFC and I missed this one. Thank you for reminder.
Comment 18 commit-hook freebsd_committer freebsd_triage 2024-09-18 08:14:51 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=5d54e2539429f348dac20428e8b5349e9d34a675

commit 5d54e2539429f348dac20428e8b5349e9d34a675
Author:     Eugene Grosbein <eugen@FreeBSD.org>
AuthorDate: 2024-09-12 19:09:28 +0000
Commit:     Eugene Grosbein <eugen@FreeBSD.org>
CommitDate: 2024-09-18 08:12:34 +0000

    MFC: ng_ipfw(4): add missing change after previous commit

    The function ng_ipfw_input() used to enjoy implicit
    32->16 bits truncation of ng_ipfw_findhook1's second argument.
    Make it explicit to recover from the breakage.

    PR:             281082
    Reported by:    Ruben van Staveren <ruben@verweg.com>
    Tested by:      Ruben van Staveren <ruben@verweg.com>
    Fixes:          20e1f207cc789a28783344614d6d1d1c639c5797

    (cherry picked from commit becd0079c052cb87e7649b78733b99abae8861ee)

 sys/netgraph/ng_ipfw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 19 commit-hook freebsd_committer freebsd_triage 2024-09-18 08:14:52 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=784817824bb4447eeda21f9ed55340def93deb29

commit 784817824bb4447eeda21f9ed55340def93deb29
Author:     Eugene Grosbein <eugen@FreeBSD.org>
AuthorDate: 2024-09-12 19:09:28 +0000
Commit:     Eugene Grosbein <eugen@FreeBSD.org>
CommitDate: 2024-09-18 08:14:06 +0000

    ng_ipfw(4): add missing change after previous commit

    The function ng_ipfw_input() used to enjoy implicit
    32->16 bits truncation of ng_ipfw_findhook1's second argument.
    Make it explicit to recover from the breakage.

    PR:             281082
    Reported by:    Ruben van Staveren <ruben@verweg.com>
    Tested by:      Ruben van Staveren <ruben@verweg.com>
    Fixes:          20e1f207cc789a28783344614d6d1d1c639c5797

    (cherry picked from commit becd0079c052cb87e7649b78733b99abae8861ee)

 sys/netgraph/ng_ipfw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)