Bug 203459 - [ipfw] [patch] userspace/kernel mismatch on checking length of src-ip/dst-ip address lists
Summary: [ipfw] [patch] userspace/kernel mismatch on checking length of src-ip/dst-ip ...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Alexander V. Chernikov
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-09-30 20:46 UTC by groos
Modified: 2017-08-24 13:59 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description groos 2015-09-30 20:46:58 UTC
The ipfw command accepts up to 31 addresses in the address list of a dst-ip or src-ip selector, but the kernel only accepts up to 15.


To reproduce:
-------------

Hitting the kernel limit:

[hub] /root # ipfw add 1 count dst-ip 1.0.0.1,1.0.0.2,1.0.0.3,1.0.0.4,1.0.0.5,1.0.0.6,1.0.0.7,1.0.0.8,1.0.0.9,1.0.0.10,1.0.0.11,1.0.0.12,1.0.0.13,1.0.0.14,1.0.0.15
00001 count ip from any to any dst-ip 1.0.0.1,1.0.0.2,1.0.0.3,1.0.0.4,1.0.0.5,1.0.0.6,1.0.0.7,1.0.0.8,1.0.0.9,1.0.0.10,1.0.0.11,1.0.0.12,1.0.0.13,1.0.0.14,1.0.0.15
[hub] /root # ipfw add 1 count dst-ip 1.0.0.1,1.0.0.2,1.0.0.3,1.0.0.4,1.0.0.5,1.0.0.6,1.0.0.7,1.0.0.8,1.0.0.9,1.0.0.10,1.0.0.11,1.0.0.12,1.0.0.13,1.0.0.14,1.0.0.15,1.0.0.16
ipfw: getsockopt(IP_FW_ADD): Invalid argument
[hub] /root # dmesg|grep ipfw
ipfw: opcode 6 size 33 wrong

Hitting the ipfw command limit:

[hub] /root # ipfw add 1 count dst-ip 1.0.0.1,1.0.0.2,1.0.0.3,1.0.0.4,1.0.0.5,1.0.0.6,1.0.0.7,1.0.0.8,1.0.0.9,1.0.0.10,1.0.0.11,1.0.0.12,1.0.0.13,1.0.0.14,1.0.0.15,1.0.0.16,1.0.0.17,1.0.0.18,1.0.0.19,1.0.0.20,1.0.0.21,1.0.0.22,1.0.0.23,1.0.0.24,1.0.0.25,1.0.0.26,1.0.0.27,1.0.0.28,1.0.0.29,1.0.0.30,1.0.0.31
ipfw: getsockopt(IP_FW_ADD): Invalid argument
[hub] /root # ipfw add 1 count dst-ip 1.0.0.1,1.0.0.2,1.0.0.3,1.0.0.4,1.0.0.5,1.0.0.6,1.0.0.7,1.0.0.8,1.0.0.9,1.0.0.10,1.0.0.11,1.0.0.12,1.0.0.13,1.0.0.14,1.0.0.15,1.0.0.16,1.0.0.17,1.0.0.18,1.0.0.19,1.0.0.20,1.0.0.21,1.0.0.22,1.0.0.23,1.0.0.24,1.0.0.25,1.0.0.26,1.0.0.27,1.0.0.28,1.0.0.29,1.0.0.30,1.0.0.31,1.0.0.32
ipfw: address list too long


Patch:
------

The following seems to fix it:

diff --git a/sys/netpfil/ipfw/ip_fw_sockopt.c b/sys/netpfil/ipfw/ip_fw_sockopt.c
index ef1ff6c..358bcf9 100644
--- a/sys/netpfil/ipfw/ip_fw_sockopt.c
+++ b/sys/netpfil/ipfw/ip_fw_sockopt.c
@@ -1515,7 +1515,7 @@ check_ipfw_rule_body(ipfw_insn *cmd, int cmd_len, struct rule_check_info *ci)
                case O_IP_SRC_MASK:
                case O_IP_DST_MASK:
                        /* only odd command lengths */
-                       if ( !(cmdlen & 1) || cmdlen > 31)
+                       if ( !(cmdlen & 1) )
                                goto bad_size;
                        break;

It looks like that '31' might be an artificial limit.  The fix allows longer lists to be loaded and they do select packets correctly as expected.
Comment 1 Alexander V. Chernikov freebsd_committer freebsd_triage 2015-10-01 13:59:48 UTC
You're right, the real kernel limit is 31 (64 u32 words in ip_insn, each ip comes with mask, so we have (64-1)/2 possible ips.

Also, 15 addresses kernel check might also be a mistake.
However, I'm not 100% sure that bumping this limit helps, because:

1) If one have a system that installs addresses inside the rule and 15 addresses is not enough, there is a good chance that after some time 31 would also be not enough (I hit it once), but the next bump is close to impossible
2) Kernel performs linear walking through this huge opcode, and in terms of performance  things are starting to be comparable to the table lookup (and tables are pretty efficient in HEAD ipfw version)

In general, tables are more scalable and easy managed, you can have a decent amount of them, the support IPv4 and IPv6 simultaneously.
So, I'd suggest to use tables instead of src-ip/dst-ip chains

However, If you really need this, could you describe your setup (maybe privately)?
Comment 2 groos 2015-10-01 14:32:06 UTC
Indeed, we will bump up against 1) soon, but for now this fix doubles one of our scaling axes in the short term and is certainly getting us out of the woods.  In the long run I will have to rework some of our fw strategy to use tables for this particular case (we already use tables extensively elsewhere).

Our ruleset is remarkably long and complex and always risky to rework, so it would be nice to have this fix in the mean time.
Comment 3 commit-hook freebsd_committer freebsd_triage 2015-10-03 05:43:25 UTC
A commit references this bug:

Author: melifaro
Date: Sat Oct  3 05:42:26 UTC 2015
New revision: 288530
URL: https://svnweb.freebsd.org/changeset/base/288530

Log:
  Bump number of prefixes in O_IP_<SRC|DST> from 15 to 31 (max possible).

  PR:		203459
  Submitted by:	groos at xiplink.com
  MFC after:	2 weeks

Changes:
  head/sys/netpfil/ipfw/ip_fw_sockopt.c
Comment 4 Mark Linimon freebsd_committer freebsd_triage 2017-08-24 13:59:34 UTC
Committed back in 2015.