* Re: [PATCH ethtool] Add command to dump module EEPROM
From: Ben Hutchings @ 2012-05-14 14:18 UTC (permalink / raw)
To: Yaniv Rosner; +Cc: David Miller, netdev, Eilon Greenstein, Stuart Hodgson
In-Reply-To: <1337008431-6304-1-git-send-email-yanivr@broadcom.com>
On Mon, 2012-05-14 at 18:13 +0300, Yaniv Rosner wrote:
> Hi Ben,
> This patch adds a new option to dump (SFP+, XFP, ...) module EEPROM following
> recent support to kernel side. Below some examples:
>
> bash-3.00# ethtool -m eth1 offset 0x14 length 32 raw on
> JDSU PLRXPLSCS432
>
> bash-3.00# ethtool -m eth1 offset 0x14 length 32
> Offset Values
> ------ ------
> 0x0014 4a 44 53 55 20 20 20 20 20 20 20 20 20 20 20 20
> 0x0024 00 00 01 9c 50 4c 52 58 50 4c 53 43 53 34 33 32
>
> Please consider applying to ethtool.
I agree there should be ASCII-hex and binary dump modes, but we should
also support decoding of recognised EEPROM types (as Stuart proposed
earlier).
> Thanks,
> Yaniv
>
> Signed-off-by: Yaniv Rosner <yanivr@broadcom.com>
> ---
> ethtool-copy.h | 312 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> ethtool.8.in | 23 ++++-
> ethtool.c | 63 +++++++++++
> 3 files changed, 393 insertions(+), 5 deletions(-)
>
> diff --git a/ethtool-copy.h b/ethtool-copy.h
> index d904c1a..604dbef 100644
> --- a/ethtool-copy.h
> +++ b/ethtool-copy.h
> @@ -13,6 +13,9 @@
> #ifndef _LINUX_ETHTOOL_H
> #define _LINUX_ETHTOOL_H
>
> +#ifdef __KERNEL__
> +#include <linux/compat.h>
> +#endif
> #include <linux/types.h>
> #include <linux/if_ether.h>
>
[...]
You've updated this wrongly; run 'make headers_install' in the kernel
tree and then copy from usr/include/ethtool.h.
[...]
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 63d5d48..470fd8d 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -325,6 +325,13 @@ ethtool \- query or control network driver and hardware settings
> .I devname flag
> .A1 on off
> .RB ...
> +.HP
> +.B ethtool \-m|\-\-mod\-eeprom\-dump
> +.I devname
> +.B2 raw on off
> +.BN offset
> +.BN length
> +.HP
> .
> .\" Adjust lines (i.e. full justification) and hyphenate.
> .ad
> @@ -800,6 +807,19 @@ Sets the device's private flags as specified.
> .I flag
> .A1 on off
> Sets the state of the named private flag.
> +.TP
> +.B \-m \-\-mod\-eeprom\-dump
> +Retrieves and prints module (SFP+, XFP, ...) EEPROMs dump for the specified network device.
> +Default is to dump the entire EEPROM.
> +.TP
> +.BI raw \ on|off
> +Dumps the raw EEPROM data to stdout.
Only true for 'raw on', not 'raw off'!
> +.TP
> +.BI offset \ N
> +Start address of module EEPROM dump.
> +.TP
> +.BI length \ N
> +Length of module EEPROM dump.
> .SH BUGS
> Not supported (in part or whole) on all network drivers.
> .SH AUTHOR
> @@ -815,7 +835,8 @@ Eli Kupermann,
> Scott Feldman,
> Andi Kleen,
> Alexander Duyck,
> -Sucheta Chakraborty.
> +Sucheta Chakraborty,
> +Yaniv Rosner.
> .SH AVAILABILITY
> .B ethtool
> is available from
> diff --git a/ethtool.c b/ethtool.c
> index e80b38b..6d022c3 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2214,6 +2214,64 @@ static int do_nway_rst(struct cmd_context *ctx)
> return err;
> }
>
> +static int do_gmoduleeeprom(struct cmd_context *ctx)
> +{
> + int geeprom_changed = 0;
> + int geeprom_dump_raw = 0;
> + u32 geeprom_offset = 0;
> + u32 geeprom_length = -1;
> + struct cmdline_info cmdline_geeprom[] = {
> + { "offset", CMDL_U32, &geeprom_offset, NULL },
> + { "length", CMDL_U32, &geeprom_length, NULL },
> + { "raw", CMDL_BOOL, &geeprom_dump_raw, NULL },
> + };
> + int err;
> + struct ethtool_modinfo modinfo;
> + struct ethtool_eeprom *eeprom;
> + struct ethtool_drvinfo drvinfo;
> +
> + parse_generic_cmdline(ctx, &geeprom_changed,
> + cmdline_geeprom, ARRAY_SIZE(cmdline_geeprom));
> +
> + drvinfo.cmd = ETHTOOL_GDRVINFO;
> + err = send_ioctl(ctx, &drvinfo);
> + if (err < 0) {
> + perror("Cannot get driver information");
> + return 74;
> + }
What is the point of running ETHTOOL_GDRVINFO?
> + modinfo.cmd = ETHTOOL_GMODULEINFO;
> + err = send_ioctl(ctx, &modinfo);
> + if (err < 0) {
> + perror("Cannot get driver information");
> + return 74;
No more magic return codes, please. Just return 1 on error. Also the
error message here seems wrong.
> + }
> +
> + if (geeprom_length == -1)
> + geeprom_length = modinfo.eeprom_len;
> +
> + if (modinfo.eeprom_len < geeprom_offset + geeprom_length)
> + geeprom_length = modinfo.eeprom_len - geeprom_offset;
> + eeprom = calloc(1, sizeof(*eeprom)+geeprom_length);
> + if (!eeprom) {
> + perror("Cannot allocate memory for EEPROM data");
> + return 75;
> + }
> + eeprom->cmd = ETHTOOL_GMODULEEEPROM;
> + eeprom->len = geeprom_length;
> + eeprom->offset = geeprom_offset;
> + err = send_ioctl(ctx, eeprom);
> + if (err < 0) {
> + perror("Cannot get EEPROM data");
> + free(eeprom);
> + return 74;
> + }
> + err = dump_eeprom(geeprom_dump_raw, &drvinfo, eeprom);
> + free(eeprom);
> +
> + return err;
> +}
> +
> static int do_geeprom(struct cmd_context *ctx)
> {
> int geeprom_changed = 0;
> @@ -3231,6 +3289,11 @@ static const struct option {
> { "--show-priv-flags" , 1, do_gprivflags, "Query private flags" },
> { "--set-priv-flags", 1, do_sprivflags, "Set private flags",
> " FLAG on|off ...\n" },
> + { "-m|--mod-eeprom-dump", 1, do_gmoduleeeprom,
> + "Dumps SFP+ module EEPROM",
As you correctly noted in the manual page, this isn't limited to SFP+.
Ben.
> + " [ raw on|off ]\n"
> + " [ offset N ]\n"
> + " [ length N ]\n" },
> { "-h|--help", 0, show_usage, "Show this help" },
> { "--version", 0, do_version, "Show version number" },
> {}
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* RE: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug
From: David Laight @ 2012-05-14 14:19 UTC (permalink / raw)
To: pablo, netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1336996023-20249-2-git-send-email-pablo@netfilter.org>
> --- a/include/linux/netfilter/ipset/ip_set_timeout.h
> +++ b/include/linux/netfilter/ipset/ip_set_timeout.h
> @@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
> {
> unsigned int timeout = ip_set_get_h32(tb);
>
> + /* Normalize to fit into jiffies */
> + if (timeout > UINT_MAX/1000)
> + timeout = UINT_MAX/1000;
> +
Doesn't that rather assume that HZ is 1000 ?
David
^ permalink raw reply
* Re: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto
From: Florian Westphal @ 2012-05-14 14:22 UTC (permalink / raw)
To: David Laight
Cc: Alban Crequy, Pablo Neira Ayuso, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B6F0A@saturn3.aculab.com>
David Laight <David.Laight@ACULAB.COM> wrote:
>
> > NFPROTO_* constants were usually equal to PF_* constants but it is not
> > necessary and it will waste less memory if we don't do so
> > (see commit 7e9c6e
> > "netfilter: Introduce NFPROTO_* constants")
> ...
> >
> > static struct nf_hook_ops dnrmg_ops __read_mostly = {
> > .hook = dnrmg_hook,
> > - .pf = PF_DECnet,
> > + .pf = NFPROTO_DECNET,
> > .hooknum = NF_DN_ROUTE,
> > .priority = NF_DN_PRI_DNRTMSG,
> > };
>
> Might it be worth renaming the .pf member to (say) .nfproto
> to help avoid confusion?
NFPROTO_* values are exported to userspace, so I don't think
its safe to change these values.
^ permalink raw reply
* Re: [PATCH net-next v3] be2net: Fix to allow get/set of debug levels in the firmware.
From: Ben Hutchings @ 2012-05-14 14:29 UTC (permalink / raw)
To: Somnath Kotur; +Cc: netdev, Suresh Redy
In-Reply-To: <847bec25-7806-4eaf-8a3e-2411a0c6bba9@exht1.ad.emulex.com>
On Sat, 2012-05-12 at 09:09 +0530, Somnath Kotur wrote:
[...]
> +static void be_set_msg_level(struct net_device *netdev, u32 level)
> +{
> + struct be_adapter *adapter = netdev_priv(netdev);
> +
> + if (lancer_chip(adapter)) {
> + dev_err(&adapter->pdev->dev, "Operation not supported\n");
> + return;
> + }
> +
> + if (adapter->msg_enable == level)
> + return;
> +
> + if (level & NETIF_MSG_HW != adapter->msg_enable & NETIF_MSG_HW)
> + be_set_fw_log_level(adapter, level & NETIF_MSG_HW ?
> + FW_LOG_LEVEL_DEFAULT : FW_LOG_LEVEL_FATAL);
[...]
Oh, come on, you can't have actually tested this. The compiler even
warns you this is probably wrong.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug
From: Pablo Neira Ayuso @ 2012-05-14 14:36 UTC (permalink / raw)
To: David Laight; +Cc: netfilter-devel, davem, netdev, Jozsef Kadlecsik
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B6F0B@saturn3.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 782 bytes --]
On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:
>
> > --- a/include/linux/netfilter/ipset/ip_set_timeout.h
> > +++ b/include/linux/netfilter/ipset/ip_set_timeout.h
> > @@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
> > {
> > unsigned int timeout = ip_set_get_h32(tb);
> >
> > + /* Normalize to fit into jiffies */
> > + if (timeout > UINT_MAX/1000)
> > + timeout = UINT_MAX/1000;
> > +
>
> Doesn't that rather assume that HZ is 1000 ?
Indeed. I overlooked that. Thanks David.
New patch attached fixing this. I've rebased my tree.
@Jozsef: BTW, why do we have
include/linux/netfilter/ipset/ip_set_timeout.h
living under include/linux ?
All definitions are private to the kernel. Why not moving that header
(and other similar) to include/net ?
[-- Attachment #2: 0001-netfilter-ipset-fix-timeout-value-overflow-bug.patch --]
[-- Type: text/x-diff, Size: 2645 bytes --]
>From bcb0e955ae5ea5acb1b59fb59e4fcb1c8364994d Mon Sep 17 00:00:00 2001
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Mon, 7 May 2012 02:35:44 +0000
Subject: [PATCH] netfilter: ipset: fix timeout value overflow bug
Large timeout parameters could result wrong timeout values due to
an overflow at msec to jiffies conversion (reported by Andreas Herz)
[ This patch was mangled by Pablo Neira Ayuso since David Laight notices
that we were using hardcode 1000 instead of HZ to calculate the timeout ]
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/linux/netfilter/ipset/ip_set_timeout.h | 4 ++++
net/netfilter/xt_set.c | 15 +++++++++++++--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/include/linux/netfilter/ipset/ip_set_timeout.h b/include/linux/netfilter/ipset/ip_set_timeout.h
index 4792320..40a85b1 100644
--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h
@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
{
unsigned int timeout = ip_set_get_h32(tb);
+ /* Normalize to fit into jiffies */
+ if (timeout > UINT_MAX/HZ)
+ timeout = UINT_MAX/HZ;
+
/* Userspace supplied TIMEOUT parameter: adjust crazy size */
return timeout == IPSET_NO_TIMEOUT ? IPSET_NO_TIMEOUT - 1 : timeout;
}
diff --git a/net/netfilter/xt_set.c b/net/netfilter/xt_set.c
index 0ec8138..15275e9 100644
--- a/net/netfilter/xt_set.c
+++ b/net/netfilter/xt_set.c
@@ -44,6 +44,14 @@ const struct ip_set_adt_opt n = { \
.cmdflags = cfs, \
.timeout = t, \
}
+#define ADT_MOPT(n, f, d, fs, cfs, t) \
+struct ip_set_adt_opt n = { \
+ .family = f, \
+ .dim = d, \
+ .flags = fs, \
+ .cmdflags = cfs, \
+ .timeout = t, \
+}
/* Revision 0 interface: backward compatible with netfilter/iptables */
@@ -296,11 +304,14 @@ static unsigned int
set_target_v2(struct sk_buff *skb, const struct xt_action_param *par)
{
const struct xt_set_info_target_v2 *info = par->targinfo;
- ADT_OPT(add_opt, par->family, info->add_set.dim,
- info->add_set.flags, info->flags, info->timeout);
+ ADT_MOPT(add_opt, par->family, info->add_set.dim,
+ info->add_set.flags, info->flags, info->timeout);
ADT_OPT(del_opt, par->family, info->del_set.dim,
info->del_set.flags, 0, UINT_MAX);
+ /* Normalize to fit into jiffies */
+ if (add_opt.timeout > UINT_MAX/HZ)
+ add_opt.timeout = UINT_MAX/HZ;
if (info->add_set.index != IPSET_INVALID_ID)
ip_set_add(info->add_set.index, skb, par, &add_opt);
if (info->del_set.index != IPSET_INVALID_ID)
--
1.7.10
^ permalink raw reply related
* Re: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto
From: Pablo Neira Ayuso @ 2012-05-14 14:38 UTC (permalink / raw)
To: David Laight
Cc: Alban Crequy, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B6F0A@saturn3.aculab.com>
On Mon, May 14, 2012 at 03:18:16PM +0100, David Laight wrote:
>
> > NFPROTO_* constants were usually equal to PF_* constants but it is not
> > necessary and it will waste less memory if we don't do so
> > (see commit 7e9c6e
> > "netfilter: Introduce NFPROTO_* constants")
> ...
> >
> > static struct nf_hook_ops dnrmg_ops __read_mostly = {
> > .hook = dnrmg_hook,
> > - .pf = PF_DECnet,
> > + .pf = NFPROTO_DECNET,
> > .hooknum = NF_DN_ROUTE,
> > .priority = NF_DN_PRI_DNRTMSG,
> > };
>
> Might it be worth renaming the .pf member to (say) .nfproto
> to help avoid confusion?
That can be done follow-up patch, I guess.
^ permalink raw reply
* Re: [PATCH] netfilter: xt_HMARK: endian bugs
From: Pablo Neira Ayuso @ 2012-05-14 14:40 UTC (permalink / raw)
To: Hans Schillstrom
Cc: kaber, jengelh, netfilter-devel, netdev, dan.carpenter, hans
In-Reply-To: <1337002943-16374-1-git-send-email-hans.schillstrom@ericsson.com>
On Mon, May 14, 2012 at 03:42:23PM +0200, Hans Schillstrom wrote:
> A mix of u32 and __be32 causes endian warning.
> Switch to __be32 and __be16 for addresses and ports.
> Added (__force u32) at some places.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
> ---
> include/linux/netfilter/xt_HMARK.h | 4 ++--
> net/netfilter/xt_HMARK.c | 35 ++++++++++++++++++-----------------
> 2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/netfilter/xt_HMARK.h b/include/linux/netfilter/xt_HMARK.h
> index abb1650..e2af67e 100644
> --- a/include/linux/netfilter/xt_HMARK.h
> +++ b/include/linux/netfilter/xt_HMARK.h
> @@ -24,8 +24,8 @@ enum {
>
> union hmark_ports {
> struct {
> - __u16 src;
> - __u16 dst;
> + __be16 src;
> + __be16 dst;
> } p16;
> __u32 v32;
> };
> diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
> index 32fbd73..38ed442 100644
> --- a/net/netfilter/xt_HMARK.c
> +++ b/net/netfilter/xt_HMARK.c
> @@ -32,13 +32,13 @@ MODULE_ALIAS("ipt_HMARK");
> MODULE_ALIAS("ip6t_HMARK");
>
> struct hmark_tuple {
> - u32 src;
> - u32 dst;
> + __be32 src;
> + __be32 dst;
> union hmark_ports uports;
> uint8_t proto;
> };
>
> -static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
> +static inline __be32 hmark_addr6_mask(const __be32 *addr32, const __be32 *mask)
> {
> return (addr32[0] & mask[0]) ^
> (addr32[1] & mask[1]) ^
> @@ -46,8 +46,8 @@ static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
> (addr32[3] & mask[3]);
> }
>
> -static inline u32
> -hmark_addr_mask(int l3num, const __u32 *addr32, const __u32 *mask)
> +static inline __be32
> +hmark_addr_mask(int l3num, const __be32 *addr32, const __be32 *mask)
> {
> switch (l3num) {
> case AF_INET:
> @@ -74,10 +74,10 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
> otuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
> rtuple = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
>
> - t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.all,
> - info->src_mask.all);
> - t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.all,
> - info->dst_mask.all);
> + t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.ip6,
> + info->src_mask.ip6);
> + t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.ip6,
> + info->dst_mask.ip6);
>
> if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
> return 0;
> @@ -88,7 +88,7 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
> t->uports.p16.dst = rtuple->src.u.all;
> t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
> info->port_set.v32;
> - if (t->uports.p16.dst < t->uports.p16.src)
> + if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
Do we really need this to make sparse happy?
> swap(t->uports.p16.dst, t->uports.p16.src);
> }
>
> @@ -103,10 +103,11 @@ hmark_hash(struct hmark_tuple *t, const struct xt_hmark_info *info)
> {
> u32 hash;
>
> - if (t->dst < t->src)
> + if (ntohl(t->dst) < ntohl(t->src))
> swap(t->src, t->dst);
>
> - hash = jhash_3words(t->src, t->dst, t->uports.v32, info->hashrnd);
> + hash = jhash_3words((__force u32) t->src, (__force u32) t->dst,
> + t->uports.v32, info->hashrnd);
> hash = hash ^ (t->proto & info->proto_mask);
>
> return (hash % info->hmodulus) + info->hoffset;
This will clash with my patch. No problem, I'll manually fix it
myself.
> @@ -129,7 +130,7 @@ hmark_set_tuple_ports(const struct sk_buff *skb, unsigned int nhoff,
> t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
> info->port_set.v32;
>
> - if (t->uports.p16.dst < t->uports.p16.src)
> + if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
> swap(t->uports.p16.dst, t->uports.p16.src);
> }
>
> @@ -178,8 +179,8 @@ hmark_pkt_set_htuple_ipv6(const struct sk_buff *skb, struct hmark_tuple *t,
> return -1;
> }
> noicmp:
> - t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.all);
> - t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.all);
> + t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.ip6);
> + t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.ip6);
>
> if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
> return 0;
> @@ -255,8 +256,8 @@ hmark_pkt_set_htuple_ipv4(const struct sk_buff *skb, struct hmark_tuple *t,
> }
> }
>
> - t->src = (__force u32) ip->saddr;
> - t->dst = (__force u32) ip->daddr;
> + t->src = ip->saddr;
> + t->dst = ip->daddr;
>
> t->src &= info->src_mask.ip;
> t->dst &= info->dst_mask.ip;
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO
From: Pablo Neira Ayuso @ 2012-05-14 14:42 UTC (permalink / raw)
To: Alban Crequy
Cc: Patrick McHardy, Vincent Sanders, Javier Martinez Canillas,
netfilter-devel, netdev
In-Reply-To: <1337003799-2517-1-git-send-email-alban.crequy@collabora.co.uk>
On Mon, May 14, 2012 at 02:56:34PM +0100, Alban Crequy wrote:
> With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
> NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
> in new protocols.
>
> Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
> ---
> net/netfilter/core.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index e1b7e05..4f16552 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> struct nf_hook_ops *elem;
> int err;
>
> + if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
> + BUG();
> + return 1;
nf_register_hook returns a negative value on error. -EINVAL can be
fine.
> + }
> +
> err = mutex_lock_interruptible(&nf_hook_mutex);
> if (err < 0)
> return err;
> --
> 1.7.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto
From: Pablo Neira Ayuso @ 2012-05-14 14:45 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Alban Crequy, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <1337003799-2517-2-git-send-email-alban.crequy@collabora.co.uk>
@Jan,
I remember you introduced all this NFPROTO_* thing time ago.
Any complain on this patchset?
Thanks.
On Mon, May 14, 2012 at 02:56:35PM +0100, Alban Crequy wrote:
> NFPROTO_* constants were usually equal to PF_* constants but it is not
> necessary and it will waste less memory if we don't do so (see commit 7e9c6e
> "netfilter: Introduce NFPROTO_* constants")
>
> Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
> ---
> net/decnet/netfilter/dn_rtmsg.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c
> index 1531135..7fb7250 100644
> --- a/net/decnet/netfilter/dn_rtmsg.c
> +++ b/net/decnet/netfilter/dn_rtmsg.c
> @@ -118,7 +118,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb)
>
> static struct nf_hook_ops dnrmg_ops __read_mostly = {
> .hook = dnrmg_hook,
> - .pf = PF_DECnet,
> + .pf = NFPROTO_DECNET,
> .hooknum = NF_DN_ROUTE,
> .priority = NF_DN_PRI_DNRTMSG,
> };
> --
> 1.7.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug
From: Eric Dumazet @ 2012-05-14 14:47 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: David Laight, netfilter-devel, davem, netdev, Jozsef Kadlecsik
In-Reply-To: <20120514143635.GA12992@1984>
On Mon, 2012-05-14 at 16:36 +0200, Pablo Neira Ayuso wrote:
> On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:
> >
> > > --- a/include/linux/netfilter/ipset/ip_set_timeout.h
> > > +++ b/include/linux/netfilter/ipset/ip_set_timeout.h
> > > @@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
> > > {
> > > unsigned int timeout = ip_set_get_h32(tb);
> > >
> > > + /* Normalize to fit into jiffies */
> > > + if (timeout > UINT_MAX/1000)
> > > + timeout = UINT_MAX/1000;
> > > +
> >
> > Doesn't that rather assume that HZ is 1000 ?
>
> Indeed. I overlooked that. Thanks David.
I dont think so.
1000 here is really MSEC_PER_SEC
(All occurrences should be changed in this file)
^ permalink raw reply
* Re: [PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO
From: Jan Engelhardt @ 2012-05-14 15:00 UTC (permalink / raw)
To: Alban Crequy
Cc: Pablo Neira Ayuso, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <1337003799-2517-1-git-send-email-alban.crequy@collabora.co.uk>
On Monday 2012-05-14 15:56, Alban Crequy wrote:
>With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
>NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
>in new protocols.
>index e1b7e05..4f16552 100644
>--- a/net/netfilter/core.c
>+++ b/net/netfilter/core.c
>@@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> struct nf_hook_ops *elem;
> int err;
>
>+ if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
>+ BUG();
>+ return 1;
>+ }
Like always, I'd prefer a WARN() instead, here paired with return -EINVAL.
Especially when the error path is (seems) simple, halting the entire machine
does not look very nice.
^ permalink raw reply
* Re: [PATCH] netfilter: xt_HMARK: endian bugs
From: Hans Schillstrom @ 2012-05-14 15:05 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: kaber@trash.net, jengelh@medozas.de,
netfilter-devel@vger.kernel.org, netdev@vger.kernel.org,
dan.carpenter@oracle.com, hans@schillstrom.com
In-Reply-To: <20120514144052.GD12992@1984>
On Monday 14 May 2012 16:40:52 Pablo Neira Ayuso wrote:
> On Mon, May 14, 2012 at 03:42:23PM +0200, Hans Schillstrom wrote:
> > A mix of u32 and __be32 causes endian warning.
> > Switch to __be32 and __be16 for addresses and ports.
> > Added (__force u32) at some places.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
> > ---
> > include/linux/netfilter/xt_HMARK.h | 4 ++--
> > net/netfilter/xt_HMARK.c | 35 ++++++++++++++++++-----------------
> > 2 files changed, 20 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/linux/netfilter/xt_HMARK.h b/include/linux/netfilter/xt_HMARK.h
> > index abb1650..e2af67e 100644
> > --- a/include/linux/netfilter/xt_HMARK.h
> > +++ b/include/linux/netfilter/xt_HMARK.h
> > @@ -24,8 +24,8 @@ enum {
> >
> > union hmark_ports {
> > struct {
> > - __u16 src;
> > - __u16 dst;
> > + __be16 src;
> > + __be16 dst;
> > } p16;
> > __u32 v32;
> > };
> > diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
> > index 32fbd73..38ed442 100644
> > --- a/net/netfilter/xt_HMARK.c
> > +++ b/net/netfilter/xt_HMARK.c
> > @@ -32,13 +32,13 @@ MODULE_ALIAS("ipt_HMARK");
> > MODULE_ALIAS("ip6t_HMARK");
> >
> > struct hmark_tuple {
> > - u32 src;
> > - u32 dst;
> > + __be32 src;
> > + __be32 dst;
> > union hmark_ports uports;
> > uint8_t proto;
> > };
> >
> > -static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
> > +static inline __be32 hmark_addr6_mask(const __be32 *addr32, const __be32 *mask)
> > {
> > return (addr32[0] & mask[0]) ^
> > (addr32[1] & mask[1]) ^
> > @@ -46,8 +46,8 @@ static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
> > (addr32[3] & mask[3]);
> > }
> >
> > -static inline u32
> > -hmark_addr_mask(int l3num, const __u32 *addr32, const __u32 *mask)
> > +static inline __be32
> > +hmark_addr_mask(int l3num, const __be32 *addr32, const __be32 *mask)
> > {
> > switch (l3num) {
> > case AF_INET:
> > @@ -74,10 +74,10 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
> > otuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
> > rtuple = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
> >
> > - t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.all,
> > - info->src_mask.all);
> > - t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.all,
> > - info->dst_mask.all);
> > + t->src = hmark_addr_mask(otuple->src.l3num, otuple->src.u3.ip6,
> > + info->src_mask.ip6);
> > + t->dst = hmark_addr_mask(otuple->src.l3num, rtuple->src.u3.ip6,
> > + info->dst_mask.ip6);
> >
> > if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
> > return 0;
> > @@ -88,7 +88,7 @@ hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
> > t->uports.p16.dst = rtuple->src.u.all;
> > t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
> > info->port_set.v32;
> > - if (t->uports.p16.dst < t->uports.p16.src)
> > + if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
>
> Do we really need this to make sparse happy?
__force is ok for Sparse,
but I realize that the mixing little and big endian machines will not work
>
> > swap(t->uports.p16.dst, t->uports.p16.src);
> > }
> >
> > @@ -103,10 +103,11 @@ hmark_hash(struct hmark_tuple *t, const struct xt_hmark_info *info)
> > {
> > u32 hash;
> >
> > - if (t->dst < t->src)
> > + if (ntohl(t->dst) < ntohl(t->src))
> > swap(t->src, t->dst);
> >
> > - hash = jhash_3words(t->src, t->dst, t->uports.v32, info->hashrnd);
> > + hash = jhash_3words((__force u32) t->src, (__force u32) t->dst,
> > + t->uports.v32, info->hashrnd);
> > hash = hash ^ (t->proto & info->proto_mask);
> >
> > return (hash % info->hmodulus) + info->hoffset;
>
> This will clash with my patch. No problem, I'll manually fix it
> myself.
Thanks
>
> > @@ -129,7 +130,7 @@ hmark_set_tuple_ports(const struct sk_buff *skb, unsigned int nhoff,
> > t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
> > info->port_set.v32;
> >
> > - if (t->uports.p16.dst < t->uports.p16.src)
> > + if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
> > swap(t->uports.p16.dst, t->uports.p16.src);
> > }
> >
> > @@ -178,8 +179,8 @@ hmark_pkt_set_htuple_ipv6(const struct sk_buff *skb, struct hmark_tuple *t,
> > return -1;
> > }
> > noicmp:
> > - t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.all);
> > - t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.all);
> > + t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.ip6);
> > + t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.ip6);
> >
> > if (info->flags & XT_HMARK_FLAG(XT_HMARK_METHOD_L3))
> > return 0;
> > @@ -255,8 +256,8 @@ hmark_pkt_set_htuple_ipv4(const struct sk_buff *skb, struct hmark_tuple *t,
> > }
> > }
> >
> > - t->src = (__force u32) ip->saddr;
> > - t->dst = (__force u32) ip->daddr;
> > + t->src = ip->saddr;
> > + t->dst = ip->daddr;
> >
> > t->src &= info->src_mask.ip;
> > t->dst &= info->dst_mask.ip;
> > --
> > 1.7.2.3
> >
--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>
^ permalink raw reply
* Re: [PATCH] netfilter: xt_HMARK: endian bugs
From: Jan Engelhardt @ 2012-05-14 15:05 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Hans Schillstrom, kaber, jengelh, netfilter-devel, netdev,
dan.carpenter, hans
In-Reply-To: <20120514144052.GD12992@1984>
On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:
>> - if (t->uports.p16.dst < t->uports.p16.src)
>> + if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
>
>Do we really need this to make sparse happy?
You need it to make *maths* happy.
Consider
384 < 65407
but
ntohs(384) > ntohs(65407)
<=> 32769 > 32767
^ permalink raw reply
* RE: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto
From: Jan Engelhardt @ 2012-05-14 15:06 UTC (permalink / raw)
To: David Laight
Cc: Alban Crequy, Pablo Neira Ayuso, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B6F0A@saturn3.aculab.com>
On Monday 2012-05-14 16:18, David Laight wrote:
>
>> NFPROTO_* constants were usually equal to PF_* constants but it is not
>> necessary and it will waste less memory if we don't do so
>> (see commit 7e9c6e
>> "netfilter: Introduce NFPROTO_* constants")
>...
>>
>> static struct nf_hook_ops dnrmg_ops __read_mostly = {
>> .hook = dnrmg_hook,
>> - .pf = PF_DECnet,
>> + .pf = NFPROTO_DECNET,
>> .hooknum = NF_DN_ROUTE,
>> .priority = NF_DN_PRI_DNRTMSG,
>> };
>
>Might it be worth renaming the .pf member to (say) .nfproto
>to help avoid confusion?
Yes that seems like a sensible thing.
^ permalink raw reply
* Re: [PATCH] [RFC] netfilter: don't assume NFPROTO_* are like PF_*
From: Jan Engelhardt @ 2012-05-14 15:09 UTC (permalink / raw)
To: Alban Crequy
Cc: Pablo Neira Ayuso, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <1337003909-2582-1-git-send-email-alban.crequy@collabora.co.uk>
On Monday 2012-05-14 15:58, Alban Crequy wrote:
>--- a/include/linux/netfilter.h
>+++ b/include/linux/netfilter.h
>@@ -62,11 +62,11 @@ enum nf_inet_hooks {
>
> enum {
> NFPROTO_UNSPEC = 0,
>- NFPROTO_IPV4 = 2,
>- NFPROTO_ARP = 3,
>- NFPROTO_BRIDGE = 7,
>- NFPROTO_IPV6 = 10,
>- NFPROTO_DECNET = 12,
>+ NFPROTO_IPV4,
>+ NFPROTO_ARP,
>+ NFPROTO_BRIDGE,
>+ NFPROTO_IPV6,
>+ NFPROTO_DECNET,
> NFPROTO_NUMPROTO,
> };
This must not be changed under any circumstances. It is exported to
and used by userspace. (Except perhaps for NFPROTO_DECNET, which
refers to a quite dead protocol that I think no user parts have ever
used NFPROTO_DECNET.) I would consider it acceptable to change the
value for NFPROTO_DECNET if Pablo joins.
^ permalink raw reply
* Re: [PATCH] netfilter: xt_HMARK: endian bugs
From: Eric Dumazet @ 2012-05-14 15:24 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Pablo Neira Ayuso, Hans Schillstrom, kaber, jengelh,
netfilter-devel, netdev, dan.carpenter, hans
In-Reply-To: <alpine.LNX.2.01.1205141702110.11548@frira.vanv.qr>
On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:
> On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:
>
> >> - if (t->uports.p16.dst < t->uports.p16.src)
> >> + if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
> >
> >Do we really need this to make sparse happy?
>
> You need it to make *maths* happy.
>
> Consider
>
> 384 < 65407
>
> but
>
> ntohs(384) > ntohs(65407)
> <=> 32769 > 32767
> --
Doesnt matter at all in this context.
Take a look at
void __skb_get_rxhash(struct sk_buff *skb)
if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
swap(...)
^ permalink raw reply
* Re: [patch] Re: qlge driver corrupting kernel memory
From: Mike Galbraith @ 2012-05-14 15:33 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo; +Cc: netdev, Jitendra Kalsaria
In-Reply-To: <1336904179.7390.16.camel@marge.simpson.net>
(add CC)
On Sun, 2012-05-13 at 12:16 +0200, Mike Galbraith wrote:
> Erm, with a quilt refresh you get the compiling version :)
>
> glge: Fix double pci_free_consistent() upon tx_ring->q allocation failure
>
> Let ql_free_tx_resources() do it's job. You are not helping.
>
> Signed-off-by: Mike Galbraith <mgalbraith@suse.de>
> ---
> drivers/net/qlge/qlge_main.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> --- a/drivers/net/qlge/qlge_main.c
> +++ b/drivers/net/qlge/qlge_main.c
> @@ -2664,11 +2664,8 @@ static int ql_alloc_tx_resources(struct
> pci_alloc_consistent(qdev->pdev, tx_ring->wq_size,
> &tx_ring->wq_base_dma);
>
> - if ((tx_ring->wq_base == NULL) ||
> - tx_ring->wq_base_dma & WQ_ADDR_ALIGN) {
> - QPRINTK(qdev, IFUP, ERR, "tx_ring alloc failed.\n");
> - return -ENOMEM;
> - }
> + if ((tx_ring->wq_base == NULL) || tx_ring->wq_base_dma & WQ_ADDR_ALIGN)
> + goto err;
> tx_ring->q =
> kmalloc(tx_ring->wq_len * sizeof(struct tx_ring_desc), GFP_KERNEL);
> if (tx_ring->q == NULL)
> @@ -2676,8 +2673,7 @@ static int ql_alloc_tx_resources(struct
>
> return 0;
> err:
> - pci_free_consistent(qdev->pdev, tx_ring->wq_size,
> - tx_ring->wq_base, tx_ring->wq_base_dma);
> + QPRINTK(qdev, IFUP, ERR, "tx_ring alloc failed.\n");
> return -ENOMEM;
> }
>
>
>
^ permalink raw reply
* Re: [PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO
From: Alban Crequy @ 2012-05-14 15:39 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Patrick McHardy, Vincent Sanders, Javier Martinez Canillas,
netfilter-devel, netdev
In-Reply-To: <20120514144235.GE12992@1984>
Le Mon, 14 May 2012 16:42:35 +0200,
Pablo Neira Ayuso <pablo@netfilter.org> a écrit :
> On Mon, May 14, 2012 at 02:56:34PM +0100, Alban Crequy wrote:
> > With the NFPROTO_* constants introduced by commit 7e9c6e
> > ("netfilter: Introduce NFPROTO_* constants"), it is too easy to
> > confuse PF_* and NFPROTO_* constants in new protocols.
> >
> > Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> > Reviewed-by: Javier Martinez Canillas
> > <javier.martinez@collabora.co.uk> Reviewed-by: Vincent Sanders
> > <vincent.sanders@collabora.co.uk> ---
> > net/netfilter/core.c | 5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> > index e1b7e05..4f16552 100644
> > --- a/net/netfilter/core.c
> > +++ b/net/netfilter/core.c
> > @@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> > struct nf_hook_ops *elem;
> > int err;
> >
> > + if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >=
> > NF_MAX_HOOKS) {
> > + BUG();
> > + return 1;
>
> nf_register_hook returns a negative value on error. -EINVAL can be
> fine.
Is it the patch you mean? Do you want me to do a series repost?
From: Alban Crequy <alban.crequy@collabora.co.uk>
netfilter: sanity checks on NFPROTO_NUMPROTO
With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
in new protocols.
Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
net/netfilter/core.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e1b7e05..ac56c5b 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
struct nf_hook_ops *elem;
int err;
+ if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
+ WARN();
+ return -EINVAL;
+ }
+
err = mutex_lock_interruptible(&nf_hook_mutex);
if (err < 0)
return err;
--
1.7.2.5
--
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
^ permalink raw reply related
* Re: [PATCH ethtool] Add command to dump module EEPROM
From: Stuart Hodgson @ 2012-05-14 15:36 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Yaniv Rosner, David Miller, netdev, Eilon Greenstein
In-Reply-To: <1337005139.2550.6.camel@bwh-desktop.uk.solarflarecom.com>
On 14/05/12 15:18, Ben Hutchings wrote:
> On Mon, 2012-05-14 at 18:13 +0300, Yaniv Rosner wrote:
>> Hi Ben,
>> This patch adds a new option to dump (SFP+, XFP, ...) module EEPROM following
>> recent support to kernel side. Below some examples:
>>
>> bash-3.00# ethtool -m eth1 offset 0x14 length 32 raw on
>> JDSU PLRXPLSCS432
>>
>> bash-3.00# ethtool -m eth1 offset 0x14 length 32
>> Offset Values
>> ------ ------
>> 0x0014 4a 44 53 55 20 20 20 20 20 20 20 20 20 20 20 20
>> 0x0024 00 00 01 9c 50 4c 52 58 50 4c 53 43 53 34 33 32
>>
>> Please consider applying to ethtool.
>
> I agree there should be ASCII-hex and binary dump modes, but we should
> also support decoding of recognised EEPROM types (as Stuart proposed
> earlier).
I have a patch to do this as well, but also parse the SFP+ EEPROM.
I need to fix it up after some of the changes to the patches that added
kernel support but can submit in the next day or so if this would be of
use.
Stu
^ permalink raw reply
* Re: [PATCH] [RFC] netfilter: don't assume NFPROTO_* are like PF_*
From: Alban Crequy @ 2012-05-14 15:42 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Pablo Neira Ayuso, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <alpine.LNX.2.01.1205141707490.11548@frira.vanv.qr>
Le Mon, 14 May 2012 17:09:54 +0200 (CEST),
Jan Engelhardt <jengelh@inai.de> a écrit :
>
> On Monday 2012-05-14 15:58, Alban Crequy wrote:
> >--- a/include/linux/netfilter.h
> >+++ b/include/linux/netfilter.h
> >@@ -62,11 +62,11 @@ enum nf_inet_hooks {
> >
> > enum {
> > NFPROTO_UNSPEC = 0,
> >- NFPROTO_IPV4 = 2,
> >- NFPROTO_ARP = 3,
> >- NFPROTO_BRIDGE = 7,
> >- NFPROTO_IPV6 = 10,
> >- NFPROTO_DECNET = 12,
> >+ NFPROTO_IPV4,
> >+ NFPROTO_ARP,
> >+ NFPROTO_BRIDGE,
> >+ NFPROTO_IPV6,
> >+ NFPROTO_DECNET,
> > NFPROTO_NUMPROTO,
> > };
>
> This must not be changed under any circumstances. It is exported to
> and used by userspace. (Except perhaps for NFPROTO_DECNET, which
> refers to a quite dead protocol that I think no user parts have ever
> used NFPROTO_DECNET.) I would consider it acceptable to change the
> value for NFPROTO_DECNET if Pablo joins.
Thanks for the review. I didn't realize it was exported to and used by
userspace. I would drop this patch then.
Alban
^ permalink raw reply
* [PATCH v2 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO
From: Alban Crequy @ 2012-05-14 16:04 UTC (permalink / raw)
To: Alban Crequy
Cc: Pablo Neira Ayuso, Patrick McHardy, Vincent Sanders,
Javier Martinez Canillas, netfilter-devel, netdev
In-Reply-To: <20120514163949.37e614f4@rainbow.cbg.collabora.co.uk>
Le Mon, 14 May 2012 16:39:49 +0100,
Alban Crequy <alban.crequy@collabora.co.uk> a écrit :
> Le Mon, 14 May 2012 16:42:35 +0200,
> Pablo Neira Ayuso <pablo@netfilter.org> a écrit :
>
> > On Mon, May 14, 2012 at 02:56:34PM +0100, Alban Crequy wrote:
> > > With the NFPROTO_* constants introduced by commit 7e9c6e
> > > ("netfilter: Introduce NFPROTO_* constants"), it is too easy to
> > > confuse PF_* and NFPROTO_* constants in new protocols.
> > >
> > > Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> > > Reviewed-by: Javier Martinez Canillas
> > > <javier.martinez@collabora.co.uk> Reviewed-by: Vincent Sanders
> > > <vincent.sanders@collabora.co.uk> ---
> > > net/netfilter/core.c | 5 +++++
> > > 1 files changed, 5 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> > > index e1b7e05..4f16552 100644
> > > --- a/net/netfilter/core.c
> > > +++ b/net/netfilter/core.c
> > > @@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> > > struct nf_hook_ops *elem;
> > > int err;
> > >
> > > + if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >=
> > > NF_MAX_HOOKS) {
> > > + BUG();
> > > + return 1;
> >
> > nf_register_hook returns a negative value on error. -EINVAL can be
> > fine.
>
> Is it the patch you mean? Do you want me to do a series repost?
Please disregard the previous patch, this is the correct one.
From: Alban Crequy <alban.crequy@collabora.co.uk>
netfilter: sanity checks on NFPROTO_NUMPROTO
With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
in new protocols.
Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
---
net/netfilter/core.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e1b7e05..7422989 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -67,6 +67,14 @@ int nf_register_hook(struct nf_hook_ops *reg)
struct nf_hook_ops *elem;
int err;
+ if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
+ WARN(reg->pf >= NFPROTO_NUMPROTO,
+ "netfilter: Invalid nfproto %d\n", reg->pf);
+ WARN(reg->hooknum >= NF_MAX_HOOKS,
+ "netfilter: Invalid hooknum %d\n", reg->hooknum);
+ return -EINVAL;
+ }
+
err = mutex_lock_interruptible(&nf_hook_mutex);
if (err < 0)
return err;
--
1.7.2.5
--
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
^ permalink raw reply related
* Re: [PATCH] netfilter: xt_HMARK: endian bugs
From: Hans Schillstrom @ 2012-05-14 16:09 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jan Engelhardt, Pablo Neira Ayuso, kaber@trash.net,
jengelh@medozas.de, netfilter-devel@vger.kernel.org,
netdev@vger.kernel.org, dan.carpenter@oracle.com,
hans@schillstrom.com
In-Reply-To: <1337009079.8512.535.camel@edumazet-glaptop>
On Monday 14 May 2012 17:24:39 Eric Dumazet wrote:
> On Mon, 2012-05-14 at 17:05 +0200, Jan Engelhardt wrote:
> > On Monday 2012-05-14 16:40, Pablo Neira Ayuso wrote:
> >
> > >> - if (t->uports.p16.dst < t->uports.p16.src)
> > >> + if (ntohs(t->uports.p16.dst) < ntohs(t->uports.p16.src))
> > >
> > >Do we really need this to make sparse happy?
> >
> > You need it to make *maths* happy.
> >
> > Consider
> >
> > 384 < 65407
> >
> > but
> >
> > ntohs(384) > ntohs(65407)
> > <=> 32769 > 32767
> > --
>
> Doesnt matter at all in this context.
This context can contain both le & be machines,
so at least in hmark it make sense
> Take a look at
>
> void __skb_get_rxhash(struct sk_buff *skb)
>
> if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
> swap(...)
>
>
--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: notify userspace of security level change
From: valdis.kletnieks @ 2012-05-14 16:22 UTC (permalink / raw)
To: Gustavo Padovan
Cc: linville, davem, linux-wireless, linux-bluetooth, linux-kernel,
netdev
In-Reply-To: <1336890007-10646-1-git-send-email-gustavo@padovan.org>
[-- Attachment #1: Type: text/plain, Size: 571 bytes --]
On Sun, 13 May 2012 03:20:07 -0300, Gustavo Padovan said:
> It enables the userspace a security level change when the socket is
> already connected and create a way to notify the socket the result of the
> request. At the moment of the request the socket is made non writable, if
> the request fails the connections closes, otherwise the socket is made
> writable again, POLL_OUT is emmited.
What happens to in-flight data already written but not yet consumed? I'm
smelling a possible race condition with data sent under the old level but
read under the new level....
[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]
^ permalink raw reply
* Re: [PATCH] netfilter: xt_HMARK: endian bugs
From: Eric Dumazet @ 2012-05-14 16:24 UTC (permalink / raw)
To: Hans Schillstrom
Cc: Jan Engelhardt, Pablo Neira Ayuso, kaber@trash.net,
jengelh@medozas.de, netfilter-devel@vger.kernel.org,
netdev@vger.kernel.org, dan.carpenter@oracle.com,
hans@schillstrom.com
In-Reply-To: <201205141809.18174.hans.schillstrom@ericsson.com>
On Mon, 2012-05-14 at 18:09 +0200, Hans Schillstrom wrote:
> This context can contain both le & be machines,
> so at least in hmark it make sense
Before jhash() and its shuffle ? What do you mean ?
Please respin your patch using (__force u16/u32) instead of
useless/expensive ntohs() / ntohl() (in _this_ context of hashing)
If you compare two 32bits values, of course they must have same
ordering, but seeding jhash() is another matter.
(Granted all calls use the same ordering of course)
sparse is great tool, but if you add useless ntohl() calls to make
sparse silent, then its probably better to not use sparse.
^ permalink raw reply
* ppp/l2tp doing oversized allocations ?
From: Dave Jones @ 2012-05-14 16:29 UTC (permalink / raw)
To: netdev; +Cc: Fedora Kernel Team
We just got this trace from reported by a Fedora user running 3.3.4
:WARNING: at mm/page_alloc.c:2204 __alloc_pages_nodemask+0x231/0x8f0()
:Call Trace:
: [<ffffffff81057abf>] warn_slowpath_common+0x7f/0xc0
: [<ffffffff81057b1a>] warn_slowpath_null+0x1a/0x20
: [<ffffffff81129671>] __alloc_pages_nodemask+0x231/0x8f0
: [<ffffffff814e84db>] ? dev_queue_xmit+0x1db/0x640
: [<ffffffff8151f210>] ? ip_forward_options+0x1f0/0x1f0
: [<ffffffff814ef7a1>] ? neigh_direct_output+0x11/0x20
: [<ffffffff81520dee>] ? ip_finish_output+0x17e/0x2f0
: [<ffffffff8151f210>] ? ip_forward_options+0x1f0/0x1f0
: [<ffffffff811608d3>] alloc_pages_current+0xa3/0x110
: [<ffffffff811254b4>] __get_free_pages+0x14/0x50
: [<ffffffff8116b99f>] kmalloc_order_trace+0x3f/0xd0
: [<ffffffff8156d137>] ? xfrm4_output_finish+0x27/0x40
: [<ffffffff8116c8c7>] __kmalloc+0x177/0x1a0
: [<ffffffff81521196>] ? ip_queue_xmit+0x156/0x400
: [<ffffffff814dab07>] pskb_expand_head+0x87/0x310
: [<ffffffff8113dbf9>] ? __mod_zone_page_state+0x49/0x50
: [<ffffffffa05e84dd>] pppol2tp_xmit+0x1ed/0x220 [l2tp_ppp]
: [<ffffffffa05d3d5b>] ppp_push+0x15b/0x650 [ppp_generic]
: [<ffffffff814dacc4>] ? pskb_expand_head+0x244/0x310
: [<ffffffff811285ab>] ? free_compound_page+0x1b/0x20
: [<ffffffff8112cff3>] ? __put_compound_page+0x23/0x30
: [<ffffffff8112d175>] ? put_compound_page+0x125/0x1c0
: [<ffffffffa05d489f>] ppp_xmit_process+0x46f/0x660 [ppp_generic]
: [<ffffffffa05d4bc8>] ppp_start_xmit+0x138/0x1d0 [ppp_generic]
: [<ffffffff814e7f62>] dev_hard_start_xmit+0x332/0x6d0
: [<ffffffff81503e2a>] sch_direct_xmit+0xfa/0x1d0
: [<ffffffff81503fa6>] __qdisc_run+0xa6/0x130
: [<ffffffff814e84be>] dev_queue_xmit+0x1be/0x640
: [<ffffffff8151f210>] ? ip_forward_options+0x1f0/0x1f0
: [<ffffffff814ef7a1>] neigh_direct_output+0x11/0x20
: [<ffffffff81520dee>] ip_finish_output+0x17e/0x2f0
: [<ffffffff81521906>] ip_output+0x66/0xa0
: [<ffffffff81521002>] ? __ip_local_out+0xa2/0xb0
: [<ffffffff815792ae>] xfrm_output_resume+0x38e/0x3f0
: [<ffffffff8116c0db>] ? kfree+0x3b/0x150
: [<ffffffff81297a10>] ? cryptd_free+0x60/0x60
: [<ffffffffa04fc110>] esp_output_done+0x30/0x40 [esp4]
: [<ffffffffa05ed774>] authenc_request_complete+0x14/0x20 [authenc]
: [<ffffffffa05ede4e>] crypto_authenc_givencrypt_done+0x2e/0x40 [authenc]
: [<ffffffff8129728c>] cryptd_blkcipher_crypt+0x5c/0x70
: [<ffffffff812972dc>] cryptd_blkcipher_encrypt+0x1c/0x20
: [<ffffffff81297a66>] cryptd_queue_worker+0x56/0x80
: [<ffffffff810747ae>] process_one_work+0x11e/0x470
: [<ffffffff810755bf>] worker_thread+0x15f/0x360
: [<ffffffff81075460>] ? manage_workers+0x230/0x230
: [<ffffffff81079da3>] kthread+0x93/0xa0
: [<ffffffff815fd2a4>] kernel_thread_helper+0x4/0x10
: [<ffffffff81079d10>] ? kthread_freezable_should_stop+0x70/0x70
: [<ffffffff815fd2a0>] ? gs_change+0x13/0x13
That WARN statement is this in the page allocator..
2197 /*
2198 * In the slowpath, we sanity check order to avoid ever trying to
2199 * reclaim >= MAX_ORDER areas which will never succeed. Callers may
2200 * be using allocators in order of preference for an area that is
2201 * too large.
2202 */
2203 if (order >= MAX_ORDER) {
2204 WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
2205 return NULL;
2206 }
Dave
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox