netdev - Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 21 Jun 2010 13:31:46 -0700 (Pacific Daylight Time)
From:	Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@...el.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
cc:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"jeff@...zik.org" <jeff@...zik.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>
Subject: Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

On Mon, 21 Jun 2010, Ben Hutchings wrote:

> On Wed, 2010-02-03 at 23:51 -0800, Jeff Kirsher wrote:
>> From: Peter Waskiewicz <peter.p.waskiewicz.jr@...el.com>
>>
>> Program underlying ethernet devices with n-tuple flow classification
>> filters.
>>
>> This also adds a new flag to ethtool_flags, allowing n-tuple
>> programming to be toggled using the set_flags call.
>
> I just noticed a problem with the implementation which makes me wonder
> whether this was tested at all:

Yes, it was tested.  We didn't hit every corner case, which I think your 
catch below is a corner case issue.  Our hardware can only do so much.

>
> [...]
>> +static struct cmdline_info cmdline_ntuple[] = {
>> +	{ "src-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
>> +	{ "src-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
>> +	{ "dst-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
>> +	{ "dst-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
>> +	{ "src-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
>> +	{ "src-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
>> +	{ "dst-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
>> +	{ "dst-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
>> +	{ "vlan", CMDL_INT, &ntuple_fs.vlan_tag, NULL },
>> +	{ "vlan-mask", CMDL_UINT, &ntuple_fs.vlan_tag_mask, NULL },
>> +	{ "user-def", CMDL_INT, &ntuple_fs.data, NULL },
>> +	{ "user-def-mask", CMDL_UINT, &ntuple_fs.data_mask, NULL },
>> +	{ "action", CMDL_INT, &ntuple_fs.action, NULL },
>> +};
> [...]
>> +                       if (mode == MODE_SNTUPLE) {
>> +                               if (!strcmp(argp[i], "flow-type")) {
>> +                                       i += 1;
>> +                                       if (i >= argc) {
>> +                                               show_usage(1);
>> +                                               break;
>> +                                       }
>> +                                       ntuple_fs.flow_type =
>> +                                                   rxflow_str_to_type(argp[i]);
>> +                                       i += 1;
>> +                                       parse_generic_cmdline(argc, argp, i,
>> +                                               &sntuple_changed,
>> +                                               cmdline_ntuple,
>> +                                               ARRAY_SIZE(cmdline_ntuple));
>> +                                       i = argc;
>> +                                       break;
>> +                               } else {
>> +                                       show_usage(1);
>> +                               }
>> +                               break;
>> +                       }
> [...]
>
> parse_generic_cmdline() will write an int for each argument defined with
> type CMDL_INT or CMDL_UINT.  But the fields in ntuple_fs are not all of
> type int (or even 32-bit) - some of them are 16-bit or 64-bit, and some
> of them are big-endian.  I also wonder whether anyone really wants to
> enter an IPv4 address as a single integer.

The assignment is broken since 'p' is an int.  That can be fixed.  Also, 
we can fix the 64-bit field.  I added the user-defined field to be 64-bit 
so that we weren't locking anyone down.  My hardware only uses 2 bytes, so 
I was only able to test that.

When this was proposed, we added the IPv4 address as a single int.  People 
seemed ok with it at the time, so we went with it.  If you have a 
different approach, please present it.

Cheers,
-PJ
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html