* Re: [endianness bug] cxgb4: mk_act_open_req() buggers ->{local,peer}_ip on big-endian hosts
From: Al Viro @ 2018-08-14 21:25 UTC (permalink / raw)
To: Rahul Lakkireddy; +Cc: ganeshgr, David Miller, netdev@vger.kernel.org
In-Reply-To: <20180806121459.GA5591@chelsio.com>
How can cxgb4/cxgb4_tc_flower.c handling of 16bit
fields possibly work on b-e? Look:
case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
switch (offset) {
case PEDIT_TCP_SPORT_DPORT:
if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
offload_pedit(fs, cpu_to_be32(val) >> 16,
cpu_to_be32(mask) >> 16,
TCP_SPORT);
OK, we are feeding two results of >> 16 (i.e. the values in
range 0..65535 from the host POV) to offload_pedit(). Which does
static void offload_pedit(struct ch_filter_specification *fs, u32 val, u32 mask,
u8 field)
{
u32 set_val = val & ~mask;
OK, it's a value in range 0..65535.
u32 offset = 0;
u8 size = 1;
int i;
for (i = 0; i < ARRAY_SIZE(pedits); i++) {
if (pedits[i].field == field) {
go until we finally find this:
PEDIT_FIELDS(TCP_, SPORT, 2, nat_fport, 0),
i.e.
{TCP_SPORT, 2, offsetof(struct ch_filter_specification, nat_fport)}
offset = pedits[i].offset;
size = pedits[i].size;
... resulting in offset = offsetof(..., nat_fport), size = 2
break;
}
}
memcpy((u8 *)fs + offset, &set_val, size);
... and we copy the first two bytes of set_val to fs->nat_fport, right?
On little-endian, assuming that val & 0xffff was 256 * V0 + V1 and
mask & 0xffff - 256 * M0 + M1, we get cpu_to_be32(val) >> 16 equal to
256 * V1 + V0, and similar for mask, resuling in set_val containing
{V0 & ~M0, V1 & ~M1, 0, 0}, with the first two bytes copied to fs->nat_fport.
Now, think what will happen on big-endian. The value in set_val has upper
16 bits all zero, no matter what - shift anything 32bit down by 16 and you'll
get that. And on big-endian that's first two bytes of memory representation,
so this memcpy() is absolutely guaranteed to set fs->nat_fport to zero.
No matter how fancy the hardware is, it can't guess what had the other two
bytes been - CPU has discarded those before the NIC had a chance to see
them.
Am I right assuming that the val is supposed to be {S1, S0, D1, D0},
with sport == S1 * 256 + S0, dport == D1 * 256 + D0? If so, the following
ought to work [== COMPLETELY UNTESTED, in other words] on l-e same as the
current code does and do the right thing on b-e. Objections?
offload_pedit() is broken for big-endian; it's actually easier to spell the
memcpy (and in case of ports - memcpy-with-byteswap) explicitly, avoiding
both the b-e problems and getting rid of a lot of LoC, including an unpleasant
macro.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index 3db969eefba9..020ca0121fb4 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -43,27 +43,6 @@
#define STATS_CHECK_PERIOD (HZ / 2)
-static struct ch_tc_pedit_fields pedits[] = {
- PEDIT_FIELDS(ETH_, DMAC_31_0, 4, dmac, 0),
- PEDIT_FIELDS(ETH_, DMAC_47_32, 2, dmac, 4),
- PEDIT_FIELDS(ETH_, SMAC_15_0, 2, smac, 0),
- PEDIT_FIELDS(ETH_, SMAC_47_16, 4, smac, 2),
- PEDIT_FIELDS(IP4_, SRC, 4, nat_fip, 0),
- PEDIT_FIELDS(IP4_, DST, 4, nat_lip, 0),
- PEDIT_FIELDS(IP6_, SRC_31_0, 4, nat_fip, 0),
- PEDIT_FIELDS(IP6_, SRC_63_32, 4, nat_fip, 4),
- PEDIT_FIELDS(IP6_, SRC_95_64, 4, nat_fip, 8),
- PEDIT_FIELDS(IP6_, SRC_127_96, 4, nat_fip, 12),
- PEDIT_FIELDS(IP6_, DST_31_0, 4, nat_lip, 0),
- PEDIT_FIELDS(IP6_, DST_63_32, 4, nat_lip, 4),
- PEDIT_FIELDS(IP6_, DST_95_64, 4, nat_lip, 8),
- PEDIT_FIELDS(IP6_, DST_127_96, 4, nat_lip, 12),
- PEDIT_FIELDS(TCP_, SPORT, 2, nat_fport, 0),
- PEDIT_FIELDS(TCP_, DPORT, 2, nat_lport, 0),
- PEDIT_FIELDS(UDP_, SPORT, 2, nat_fport, 0),
- PEDIT_FIELDS(UDP_, DPORT, 2, nat_lport, 0),
-};
-
static struct ch_tc_flower_entry *allocate_flower_entry(void)
{
struct ch_tc_flower_entry *new = kzalloc(sizeof(*new), GFP_KERNEL);
@@ -306,81 +285,63 @@ static int cxgb4_validate_flow_match(struct net_device *dev,
return 0;
}
-static void offload_pedit(struct ch_filter_specification *fs, u32 val, u32 mask,
- u8 field)
-{
- u32 set_val = val & ~mask;
- u32 offset = 0;
- u8 size = 1;
- int i;
-
- for (i = 0; i < ARRAY_SIZE(pedits); i++) {
- if (pedits[i].field == field) {
- offset = pedits[i].offset;
- size = pedits[i].size;
- break;
- }
- }
- memcpy((u8 *)fs + offset, &set_val, size);
-}
-
-static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
- u32 mask, u32 offset, u8 htype)
+static void process_pedit_field(struct ch_filter_specification *fs, __be32 val,
+ __be32 mask, u32 offset, u8 htype)
{
+ val &= ~mask;
switch (htype) {
case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
switch (offset) {
case PEDIT_ETH_DMAC_31_0:
fs->newdmac = 1;
- offload_pedit(fs, val, mask, ETH_DMAC_31_0);
+ memcpy(fs->dmac, &val, 4);
break;
case PEDIT_ETH_DMAC_47_32_SMAC_15_0:
if (~mask & PEDIT_ETH_DMAC_MASK)
- offload_pedit(fs, val, mask, ETH_DMAC_47_32);
+ memcpy(fs->dmac + 4, &val, 2);
else
- offload_pedit(fs, val >> 16, mask >> 16,
- ETH_SMAC_15_0);
+ memcpy(fs->smac, (__be16 *)&val + 1, 2);
break;
case PEDIT_ETH_SMAC_47_16:
fs->newsmac = 1;
- offload_pedit(fs, val, mask, ETH_SMAC_47_16);
+ memcpy(fs->smac + 2, &val, 4);
}
break;
case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
switch (offset) {
case PEDIT_IP4_SRC:
- offload_pedit(fs, val, mask, IP4_SRC);
+ memcpy(fs->nat_fip, &val, 4);
break;
case PEDIT_IP4_DST:
- offload_pedit(fs, val, mask, IP4_DST);
+ memcpy(fs->nat_lip, &val, 4);
}
fs->nat_mode = NAT_MODE_ALL;
break;
case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
switch (offset) {
case PEDIT_IP6_SRC_31_0:
- offload_pedit(fs, val, mask, IP6_SRC_31_0);
+ memcpy(fs->nat_fip, &val, 4);
break;
case PEDIT_IP6_SRC_63_32:
- offload_pedit(fs, val, mask, IP6_SRC_63_32);
+ memcpy(fs->nat_fip + 4, &val, 4);
break;
case PEDIT_IP6_SRC_95_64:
- offload_pedit(fs, val, mask, IP6_SRC_95_64);
+ memcpy(fs->nat_fip + 8, &val, 4);
break;
case PEDIT_IP6_SRC_127_96:
- offload_pedit(fs, val, mask, IP6_SRC_127_96);
+ memcpy(fs->nat_fip + 12, &val, 4);
break;
case PEDIT_IP6_DST_31_0:
- offload_pedit(fs, val, mask, IP6_DST_31_0);
+ memcpy(fs->nat_lip, &val, 4);
break;
case PEDIT_IP6_DST_63_32:
- offload_pedit(fs, val, mask, IP6_DST_63_32);
+ memcpy(fs->nat_lip + 4, &val, 4);
break;
case PEDIT_IP6_DST_95_64:
- offload_pedit(fs, val, mask, IP6_DST_95_64);
+ memcpy(fs->nat_lip + 8, &val, 4);
break;
case PEDIT_IP6_DST_127_96:
- offload_pedit(fs, val, mask, IP6_DST_127_96);
+ memcpy(fs->nat_lip + 12, &val, 4);
}
fs->nat_mode = NAT_MODE_ALL;
break;
@@ -388,12 +349,9 @@ static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
switch (offset) {
case PEDIT_TCP_SPORT_DPORT:
if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
- offload_pedit(fs, cpu_to_be32(val) >> 16,
- cpu_to_be32(mask) >> 16,
- TCP_SPORT);
+ fs->nat_fport = be16_to_cpup((__be16 *)&val);
else
- offload_pedit(fs, cpu_to_be32(val),
- cpu_to_be32(mask), TCP_DPORT);
+ fs->nat_lport = be16_to_cpup((__be16 *)&val + 1);
}
fs->nat_mode = NAT_MODE_ALL;
break;
@@ -401,12 +359,9 @@ static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
switch (offset) {
case PEDIT_UDP_SPORT_DPORT:
if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
- offload_pedit(fs, cpu_to_be32(val) >> 16,
- cpu_to_be32(mask) >> 16,
- UDP_SPORT);
+ fs->nat_fport = be16_to_cpup((__be16 *)&val);
else
- offload_pedit(fs, cpu_to_be32(val),
- cpu_to_be32(mask), UDP_DPORT);
+ fs->nat_lport = be16_to_cpup((__be16 *)&val + 1);
}
fs->nat_mode = NAT_MODE_ALL;
}
@@ -453,7 +408,8 @@ static void cxgb4_process_flow_actions(struct net_device *in,
break;
}
} else if (is_tcf_pedit(a)) {
- u32 mask, val, offset;
+ __be32 mask, val;
+ u32 offset;
int nkeys, i;
u8 htype;
@@ -471,23 +427,18 @@ static void cxgb4_process_flow_actions(struct net_device *in,
}
}
-static bool valid_l4_mask(u32 mask)
+static bool valid_l4_mask(__be32 mask)
{
- u16 hi, lo;
-
- /* Either the upper 16-bits (SPORT) OR the lower
- * 16-bits (DPORT) can be set, but NOT BOTH.
+ /* Either the SPORT OR DPORT can be set, but NOT BOTH.
*/
- hi = (mask >> 16) & 0xFFFF;
- lo = mask & 0xFFFF;
-
- return hi && lo ? false : true;
+ return !(mask && htonl(0xffff)) || !(mask & htonl(0xffff0000));
}
static bool valid_pedit_action(struct net_device *dev,
const struct tc_action *a)
{
- u32 mask, offset;
+ __be32 mask;
+ u32 offset;
u8 cmd, htype;
int nkeys, i;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
index 050c8a50ae41..4da5267726a9 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
@@ -54,44 +54,8 @@ struct ch_tc_flower_entry {
u32 filter_id;
};
-enum {
- ETH_DMAC_31_0, /* dmac bits 0.. 31 */
- ETH_DMAC_47_32, /* dmac bits 32..47 */
- ETH_SMAC_15_0, /* smac bits 0.. 15 */
- ETH_SMAC_47_16, /* smac bits 16..47 */
-
- IP4_SRC, /* 32-bit IPv4 src */
- IP4_DST, /* 32-bit IPv4 dst */
-
- IP6_SRC_31_0, /* src bits 0.. 31 */
- IP6_SRC_63_32, /* src bits 63.. 32 */
- IP6_SRC_95_64, /* src bits 95.. 64 */
- IP6_SRC_127_96, /* src bits 127..96 */
-
- IP6_DST_31_0, /* dst bits 0.. 31 */
- IP6_DST_63_32, /* dst bits 63.. 32 */
- IP6_DST_95_64, /* dst bits 95.. 64 */
- IP6_DST_127_96, /* dst bits 127..96 */
-
- TCP_SPORT, /* 16-bit TCP sport */
- TCP_DPORT, /* 16-bit TCP dport */
-
- UDP_SPORT, /* 16-bit UDP sport */
- UDP_DPORT, /* 16-bit UDP dport */
-};
-
-struct ch_tc_pedit_fields {
- u8 field;
- u8 size;
- u32 offset;
-};
-
-#define PEDIT_FIELDS(type, field, size, fs_field, offset) \
- { type## field, size, \
- offsetof(struct ch_filter_specification, fs_field) + (offset) }
-
-#define PEDIT_ETH_DMAC_MASK 0xffff
-#define PEDIT_TCP_UDP_SPORT_MASK 0xffff
+#define PEDIT_ETH_DMAC_MASK htonl(0xffff0000)
+#define PEDIT_TCP_UDP_SPORT_MASK htonl(0xffff0000)
#define PEDIT_ETH_DMAC_31_0 0x0
#define PEDIT_ETH_DMAC_47_32_SMAC_15_0 0x4
#define PEDIT_ETH_SMAC_47_16 0x8
^ permalink raw reply related
* Re: [PATCH 1/1] NFC: Fix the number of pipes
From: Kees Cook @ 2018-08-14 23:47 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Security Officers, Dan Carpenter, Kevin Deus, Samuel Ortiz,
David S. Miller, linux-wireless, Network Development, LKML
In-Reply-To: <20180814223519.13610-1-surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Tue, Aug 14, 2018 at 3:35 PM, Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> According to ETSI TS 102 622 specification chapter 4.4 pipe identifier
> is 7 bits long which allows for 128 unique pipe IDs. Because
> NFC_HCI_MAX_PIPES is used as the number of pipes supported and not
> as the max pipe ID, its value should be 128 instead of 127.
>
> nfc_hci_recv_from_llc extracts pipe ID from packet header using
> NFC_HCI_FRAGMENT(0x7F) mask which allows for pipe ID value of 127.
> Same happens when NCI_HCP_MSG_GET_PIPE() is being used. With
> pipes array having only 127 elements and pipe ID of 127 the OOB memory
> access will result.
>
> Suggested-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
-Kees
> ---
> include/net/nfc/hci.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/nfc/hci.h b/include/net/nfc/hci.h
> index 316694dafa5b..008f466d1da7 100644
> --- a/include/net/nfc/hci.h
> +++ b/include/net/nfc/hci.h
> @@ -87,7 +87,7 @@ struct nfc_hci_pipe {
> * According to specification 102 622 chapter 4.4 Pipes,
> * the pipe identifier is 7 bits long.
> */
> -#define NFC_HCI_MAX_PIPES 127
> +#define NFC_HCI_MAX_PIPES 128
> struct nfc_hci_init_data {
> u8 gate_count;
> struct nfc_hci_gate gates[NFC_HCI_MAX_CUSTOM_GATES];
> --
> 2.18.0.865.gffc8e1a3cd6-goog
>
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH v3] Add BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES bpf(2) command
From: Daniel Colascione @ 2018-08-14 20:37 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Jakub Kicinski, Joel Fernandes, linux-kernel,
Tim Murray, netdev, Lorenzo Colitti, Chenbo Feng,
Mathieu Desnoyers, Alexei Starovoitov
In-Reply-To: <20180810225246.3d3pa5qvbtoh42bt@ast-mbp.dhcp.thefacebook.com>
On Fri, Aug 10, 2018 at 3:52 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Tue, Jul 31, 2018 at 02:36:39AM -0700, Daniel Colascione wrote:
>>
>> > An API command name
>> > such as BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is simply non-generic, and
>> > exposes specific map details (here: map-in-map) into the UAPI whereas it
>> > should reside within a specific implementation instead similar to other ops
>> > we have for maps.
>>
>> But synchronize isn't conceptually a command that applies to a
>> specific map. It waits on all references. Did you address my point
>> about your proposed map-specific interface requiring redundant
>> synchronize_rcu calls in the case where we swap multiple maps and want
>> to wait for all the references to drain? Under my proposal, you'd just
>> BPF_SYNCHRONIZE_WHATEVER and call schedule_rcu once. Under your
>> proposal, we'd make it a per-map operation, so we'd issue one
>> synchronize_rcu per map.
>
> optimizing for multi-map sync sounds like premature optimization.
Maybe, but the per-map proposal is less efficient *and* more
complicated! I don't want to spend more code just to go slower.
> I believe the only issue being discussed is user space doesn't know
> when it's ok to start draining the inner map when it was replaced
> by bpf_map_update syscall command with another map, right?
Yes.
> If we agree on that, should bpf_map_update handle it then?
> Wouldn't it be much easier to understand and use from user pov?
> No new commands to learn. map_update syscall replaced the map
> and old map is no longer accessed by the program via this given map-in-map.
Maybe with a new BPF_SYNCHRONIZE flag for BPF_MAP_UPDATE_ELEM and
BPF_MAP_DELETE_ELEM. Otherwise, it seems wrong to make every user of
these commands pay for synchronization that only a few will need.
> But if the replaced map is used directly or it sits in some other
> map-in-map slot the progs can still access it.
>
> My issue with DanielC SYNC cmd that it exposes implementation details
What implementation details? The command semantics are defined
entirely in terms of existing user-visible primitives.
> and introduces complex 'synchronization' semantics. To majority of
> the users it won't be obvious what is being synchronized.
>
> My issue with DanielB WAIT_REF map_fd cmd that it needs to wait for all refs
> to this map to be dropped. I think combination of usercnt and refcnt
> can answer that, but feels dangerous to sleep potentially forever
> in a syscall until all prog->map references are gone, though such
> cmd is useful beyond map-in-map use case.
In what scenarios?
In any case, can we submit _something_?
^ permalink raw reply
* Re: [RFC PATCH v1 0/3] device property: Support MAC address in VPD
From: Florian Fainelli @ 2018-08-14 23:00 UTC (permalink / raw)
To: Brian Norris, Rob Herring, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Andrew Lunn, Dmitry Torokhov, Guenter Roeck, netdev, devicetree,
linux-kernel, Julius Werner, Stephen Boyd, Brian Norris
In-Reply-To: <20180814223758.117433-1-briannorris@chromium.org>
On 08/14/2018 03:37 PM, Brian Norris wrote:
> Hi all,
>
> Preface: I'm sure there are at least a few minor issues with this
> patchset as-is. But I'd appreciate ("RFC") if I can get general feedback
> on the approach here; perhaps there are alternatives, or perhaps I've
> missed similar proposals in the past. (My problems don't feel all that
> unique.)
>
> ---
>
> Today, we have generic support for 'mac-address' and 'local-mac-address'
> properties in both Device Tree nodes and in generic Device Properties,
> such that network device drivers can pick up a hardware address from
> there, in cases where the MAC address isn't baked into the network card.
> This method of MAC address retrieval presumes that either:
> (a) there's a unique device tree (or similar) stored on a given device
> or
> (b) some other entity (e.g., boot firmware) will modify device nodes
> runtime to place that MAC address into the appropriate device
> properties.
>
> Option (a) is not feasbile for many systems.
>
> Option (b) can work, but there are some reasons why one might not want
> to do that:
> (1) This requires that system firmware understand the device tree
> structure, sometimes to the point of memorizing path names (e.g.,
> /soc/wifi@xxxxxxxx). At least for Device Tree, these path names are
> not necessarily an ABI, and so this introduces unneeded fragility.
The path to a node is something that is well defined and should be
stable given that the high level function of the node and its unit
address are not supposed to change. Under which circumstances, besides
incorrect specification of either of these two things, do they not
consist an ABI? Not refuting your statement here, just curious when/how
this can happen?
Also, aliases in DT are meant to provide some stability.
> (2) Other than this device-tree shim requirement, system firmware may
> have no reason to understand anything about network devices.
>
> So instead, I'm looking for a way to have a device node describe where
> to find its MAC address, rather than having the device node contain the
> MAC address directly. Then system firmware doesn't have to manage
> anything.
>
> In particular, I add support for the Google Vital Product Data (VPD)
> format, used within the Coreboot project. The format is described here:
>
> https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md
>
> TL;DR: VPD consists of a TLV-like table, with key/value pairs of
> strings. This is often stored persistently on the boot flash and
> presented via in-memory Coreboot tables, for the operating system to
> read.
>
> We already have a VPD driver that parses this table and presents it to
> user space. This series extends that driver to allow in-kernel lookups
> of MAC address entries.
A possible alternative approach is to have the VPD driver become a NVMEM
producer to expose the VPD keys, did you look into that and possibly
found that it was not a good model? The downside to that approach though
is that you might have to have a phandle for the VPD provider in the
Device Tree, but AFAICS this should solve your needs?
[1]: https://patchwork.ozlabs.org/cover/956062/
[2]: https://lkml.org/lkml/2018/3/24/312
>
> Thanks,
> Brian
>
>
> Brian Norris (3):
> dt-bindings: net: Add 'mac-address-lookup' property
> device property: Support complex MAC address lookup
> firmware: vpd: add MAC address parser
>
> .../devicetree/bindings/net/ethernet.txt | 12 +++
> drivers/base/property.c | 83 ++++++++++++++++++-
> drivers/firmware/google/vpd.c | 67 +++++++++++++++
> include/linux/property.h | 23 +++++
> 4 files changed, 183 insertions(+), 2 deletions(-)
>
--
Florian
^ permalink raw reply
* [RFC PATCH v1 3/3] firmware: vpd: add MAC address parser
From: Brian Norris @ 2018-08-14 22:37 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Andrew Lunn, Florian Fainelli, Dmitry Torokhov, Guenter Roeck,
netdev, devicetree, linux-kernel, Julius Werner, Stephen Boyd,
Brian Norris
In-Reply-To: <20180814223758.117433-1-briannorris@chromium.org>
Device trees can now specify their MAC address via something like:
mac-address-lookup = "vpd:wifi_mac0";
instead of requiring boot firmware to parse and splice the FDT itself.
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/firmware/google/vpd.c | 67 +++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index e9db895916c3..b2b08497db32 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -16,6 +16,7 @@
*/
#include <linux/ctype.h>
+#include <linux/if_ether.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
@@ -24,6 +25,7 @@
#include <linux/module.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
@@ -173,6 +175,36 @@ static ssize_t vpd_section_read(struct file *filp, struct kobject *kobp,
sec->bin_attr.size);
}
+static const char *__vpd_get_entry(struct vpd_section *sec, const char *key)
+{
+ struct vpd_attrib_info *info;
+
+ list_for_each_entry(info, &sec->attribs, list)
+ if (!strcmp(info->key, key))
+ return info->value;
+
+ return NULL;
+}
+
+static const char *vpd_get_entry(const char *key)
+{
+ const char *value;
+
+ if (rw_vpd.enabled) {
+ value = __vpd_get_entry(&rw_vpd, key);
+ if (value)
+ return value;
+ }
+
+ if (ro_vpd.enabled) {
+ value = __vpd_get_entry(&ro_vpd, key);
+ if (value)
+ return value;
+ }
+
+ return NULL;
+}
+
static int vpd_section_create_attribs(struct vpd_section *sec)
{
s32 consumed;
@@ -286,6 +318,37 @@ static int vpd_sections_init(phys_addr_t physaddr)
return 0;
}
+/*
+ * 'key' is typically something like 'wifi_mac0' or 'ether_mac1'. Values are 12
+ * character strings of MAC address digits, in hex.
+ */
+static int vpd_lookup_mac_address(const char *key, u8 *mac)
+{
+ const char *entry;
+ int ret;
+
+ if (!key)
+ return -EINVAL;
+
+ entry = vpd_get_entry(key);
+ if (!entry)
+ return -ENOENT;
+
+ if (strlen(entry) != ETH_ALEN * 2)
+ return -EINVAL;
+
+ ret = hex2bin(mac, entry, ETH_ALEN);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static struct device_mac_addr_provider vpd_mac_addr_provider = {
+ .prefix = "google-vpd",
+ .lookup = &vpd_lookup_mac_address,
+};
+
static int vpd_probe(struct coreboot_device *dev)
{
int ret;
@@ -300,11 +363,15 @@ static int vpd_probe(struct coreboot_device *dev)
return ret;
}
+ device_register_mac_addr_provider(&vpd_mac_addr_provider);
+
return 0;
}
static int vpd_remove(struct coreboot_device *dev)
{
+ device_unregister_mac_addr_provider(&vpd_mac_addr_provider);
+
vpd_section_destroy(&ro_vpd);
vpd_section_destroy(&rw_vpd);
--
2.18.0.865.gffc8e1a3cd6-goog
^ permalink raw reply related
* [RFC PATCH v1 2/3] device property: Support complex MAC address lookup
From: Brian Norris @ 2018-08-14 22:37 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Andrew Lunn, Florian Fainelli, Dmitry Torokhov, Guenter Roeck,
netdev, devicetree, linux-kernel, Julius Werner, Stephen Boyd,
Brian Norris
In-Reply-To: <20180814223758.117433-1-briannorris@chromium.org>
Some firmwares include data tables that can be used to derive a MAC
address for devices on the system, but those firmwares don't directly
stash the MAC address in a device property. Support having other drivers
register lookup functions, where the device property contains
"<format>:<key>" strings; a lookup driver can register support for
handling "<format>", and "<key>" can be used by the format parser to
identify which MAC address is being requested.
This is particularly useful for the Google Vital Product Data (VPD)
format [1], which holds various product-specific key/value pairs, often
stashed in the boot flash.
[1] Ref:
https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/base/property.c | 83 +++++++++++++++++++++++++++++++++++++++-
include/linux/property.h | 23 +++++++++++
2 files changed, 104 insertions(+), 2 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..fae3390fc56c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -10,6 +10,7 @@
#include <linux/acpi.h>
#include <linux/export.h>
#include <linux/kernel.h>
+#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_graph.h>
@@ -1264,6 +1265,78 @@ static void *fwnode_get_mac_addr(struct fwnode_handle *fwnode,
return NULL;
}
+static LIST_HEAD(mac_addr_providers);
+static DEFINE_MUTEX(mac_addr_providers_mutex);
+
+void device_register_mac_addr_provider(struct device_mac_addr_provider *prov)
+{
+ mutex_lock(&mac_addr_providers_mutex);
+ list_add(&prov->entry, &mac_addr_providers);
+ mutex_unlock(&mac_addr_providers_mutex);
+}
+EXPORT_SYMBOL(device_register_mac_addr_provider);
+
+void device_unregister_mac_addr_provider(struct device_mac_addr_provider *prov)
+{
+ struct device_mac_addr_provider *p;
+
+ mutex_lock(&mac_addr_providers_mutex);
+ list_for_each_entry(p, &mac_addr_providers, entry) {
+ if (p == prov) {
+ list_del(&p->entry);
+ goto out;
+ }
+ }
+
+out:
+ mutex_unlock(&mac_addr_providers_mutex);
+}
+EXPORT_SYMBOL(device_unregister_mac_addr_provider);
+
+static void *fwnode_lookup_mac_addr(struct fwnode_handle *fwnode,
+ char *addr, int alen)
+{
+ struct device_mac_addr_provider *prov;
+ const char *prop, *sep;
+ u8 mac[ETH_ALEN];
+ int ret;
+
+ ret = fwnode_property_read_string(fwnode, "mac-address-lookup", &prop);
+ if (ret)
+ return NULL;
+
+ sep = strchr(prop, ':');
+ if (!sep)
+ return NULL;
+
+ if (alen != ETH_ALEN)
+ return NULL;
+
+ mutex_lock(&mac_addr_providers_mutex);
+ list_for_each_entry(prov, &mac_addr_providers, entry) {
+ if (strncmp(prov->prefix, prop, strlen(prov->prefix)))
+ continue;
+
+ if (prop + strlen(prov->prefix) != sep)
+ continue;
+
+ ret = prov->lookup(sep + 1, mac);
+ if (ret)
+ continue;
+
+ if (!is_valid_ether_addr(mac))
+ continue;
+
+ ether_addr_copy(addr, mac);
+
+ mutex_unlock(&mac_addr_providers_mutex);
+ return addr;
+ }
+ mutex_unlock(&mac_addr_providers_mutex);
+
+ return NULL;
+}
+
/**
* fwnode_get_mac_address - Get the MAC from the firmware node
* @fwnode: Pointer to the firmware node
@@ -1274,7 +1347,9 @@ static void *fwnode_get_mac_addr(struct fwnode_handle *fwnode,
* checked first, because that is supposed to contain to "most recent" MAC
* address. If that isn't set, then 'local-mac-address' is checked next,
* because that is the default address. If that isn't set, then the obsolete
- * 'address' is checked, just in case we're using an old device tree.
+ * 'address' is checked, just in case we're using an old device tree. And
+ * finally, we check for a method of indirect MAC address lookup, via
+ * 'mac-address-lookup'.
*
* Note that the 'address' property is supposed to contain a virtual address of
* the register set, but some DTS files have redefined that property to be the
@@ -1299,7 +1374,11 @@ void *fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr, int alen)
if (res)
return res;
- return fwnode_get_mac_addr(fwnode, "address", addr, alen);
+ res = fwnode_get_mac_addr(fwnode, "address", addr, alen);
+ if (res)
+ return res;
+
+ return fwnode_lookup_mac_addr(fwnode, addr, alen);
}
EXPORT_SYMBOL(fwnode_get_mac_address);
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..aca5dbb51e19 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -14,6 +14,7 @@
#define _LINUX_PROPERTY_H_
#include <linux/fwnode.h>
+#include <linux/list.h>
#include <linux/types.h>
struct device;
@@ -285,6 +286,28 @@ const void *device_get_match_data(struct device *dev);
int device_get_phy_mode(struct device *dev);
+/**
+ * struct device_mac_addr_provider - MAC address provider
+ *
+ * Provide methods by which the rest of the kernel can retrieve MAC addresses,
+ * e.g., from a firmware table.
+ *
+ * @entry: list head, for keeping track of all providers
+ * @prefix: string which uniquely identifies the provider
+ * @lookup: Look up a MAC address by key. The provider may associate this @key
+ * with a stored MAC address; if a match is found, the provider copies the
+ * associated MAC address to @mac. If not found, a non-zero error code is
+ * returned.
+ */
+struct device_mac_addr_provider {
+ struct list_head entry;
+ const char *prefix;
+ int (*lookup)(const char *key, u8 *mac);
+};
+
+void device_register_mac_addr_provider(struct device_mac_addr_provider *prov);
+void device_unregister_mac_addr_provider(struct device_mac_addr_provider *prov);
+
void *device_get_mac_address(struct device *dev, char *addr, int alen);
int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
--
2.18.0.865.gffc8e1a3cd6-goog
^ permalink raw reply related
* [RFC PATCH v1 1/3] dt-bindings: net: Add 'mac-address-lookup' property
From: Brian Norris @ 2018-08-14 22:37 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Andrew Lunn, Florian Fainelli, Dmitry Torokhov, Guenter Roeck,
netdev, devicetree, linux-kernel, Julius Werner, Stephen Boyd,
Brian Norris
In-Reply-To: <20180814223758.117433-1-briannorris@chromium.org>
Some firmwares present data tables that can be parsed to retrieve
device-specific details, like MAC addresses. While in some cases, one
could teach the firmware to understand the device tree format and insert
a 'mac-address'/'local-mac-address' property into the FDT on its own,
this method can be brittle (e.g., involving memorizing expected FDT
structure), and it's not strictly necessary -- especially when parsers
for such firmware formats are already needed in the OS for other
reasons.
One such format: the Vital Product Data (VPD) [1] used by Coreboot. It
supports a table of key/value pairs, and some systems keep MAC addresses
there in a well-known format. Allow a device tree to specify
(1) that the MAC address for a given device is stored in the VPD table
and
(2) what key should be used to retrieve the MAC address for said
device (e.g., "ethernet_mac0" or "wifi_mac1").
[1] Ref:
https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md
TL;DR: VPD consists of a TLV-like table, with key/value pairs of
strings. This is often stored persistently on the boot flash and
presented via in-memory Coreboot tables, for the operating system to
read.
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Documentation/devicetree/bindings/net/ethernet.txt | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index cfc376bc977a..d3fd1da18bf4 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -4,6 +4,18 @@ NOTE: All 'phy*' properties documented below are Ethernet specific. For the
generic PHY 'phys' property, see
Documentation/devicetree/bindings/phy/phy-bindings.txt.
+- mac-address-lookup: string, indicating a method by which a MAC address may be
+ discovered for this device. Methods may be parameterized by some value, such
+ that the method can determine the device's MAC address using that parameter.
+ For example, a firmware might store MAC addresses in a table, keyed by some
+ predetermined string, and baked in read-only flash. A lookup method "foo"
+ with a parameter "bar" should be written "foo:bar".
+ Supported values for method:
+ "google-vpd" - Google's Vital Product Data (VPD), as used in the Coreboot
+ project. Documentation for VPD can be found at:
+ https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md
+ Example:
+ mac-address-lookup = "google-vpd:ethernet_mac0"
- local-mac-address: array of 6 bytes, specifies the MAC address that was
assigned to the network device;
- mac-address: array of 6 bytes, specifies the MAC address that was last used by
--
2.18.0.865.gffc8e1a3cd6-goog
^ permalink raw reply related
* [RFC PATCH v1 0/3] device property: Support MAC address in VPD
From: Brian Norris @ 2018-08-14 22:37 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Andrew Lunn, Florian Fainelli, Dmitry Torokhov, Guenter Roeck,
netdev, devicetree, linux-kernel, Julius Werner, Stephen Boyd,
Brian Norris
Hi all,
Preface: I'm sure there are at least a few minor issues with this
patchset as-is. But I'd appreciate ("RFC") if I can get general feedback
on the approach here; perhaps there are alternatives, or perhaps I've
missed similar proposals in the past. (My problems don't feel all that
unique.)
---
Today, we have generic support for 'mac-address' and 'local-mac-address'
properties in both Device Tree nodes and in generic Device Properties,
such that network device drivers can pick up a hardware address from
there, in cases where the MAC address isn't baked into the network card.
This method of MAC address retrieval presumes that either:
(a) there's a unique device tree (or similar) stored on a given device
or
(b) some other entity (e.g., boot firmware) will modify device nodes
runtime to place that MAC address into the appropriate device
properties.
Option (a) is not feasbile for many systems.
Option (b) can work, but there are some reasons why one might not want
to do that:
(1) This requires that system firmware understand the device tree
structure, sometimes to the point of memorizing path names (e.g.,
/soc/wifi@xxxxxxxx). At least for Device Tree, these path names are
not necessarily an ABI, and so this introduces unneeded fragility.
(2) Other than this device-tree shim requirement, system firmware may
have no reason to understand anything about network devices.
So instead, I'm looking for a way to have a device node describe where
to find its MAC address, rather than having the device node contain the
MAC address directly. Then system firmware doesn't have to manage
anything.
In particular, I add support for the Google Vital Product Data (VPD)
format, used within the Coreboot project. The format is described here:
https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/README.md
TL;DR: VPD consists of a TLV-like table, with key/value pairs of
strings. This is often stored persistently on the boot flash and
presented via in-memory Coreboot tables, for the operating system to
read.
We already have a VPD driver that parses this table and presents it to
user space. This series extends that driver to allow in-kernel lookups
of MAC address entries.
Thanks,
Brian
Brian Norris (3):
dt-bindings: net: Add 'mac-address-lookup' property
device property: Support complex MAC address lookup
firmware: vpd: add MAC address parser
.../devicetree/bindings/net/ethernet.txt | 12 +++
drivers/base/property.c | 83 ++++++++++++++++++-
drivers/firmware/google/vpd.c | 67 +++++++++++++++
include/linux/property.h | 23 +++++
4 files changed, 183 insertions(+), 2 deletions(-)
--
2.18.0.865.gffc8e1a3cd6-goog
^ permalink raw reply
* [PATCH 1/1] NFC: Fix the number of pipes
From: Suren Baghdasaryan @ 2018-08-14 22:35 UTC (permalink / raw)
Cc: security-DgEjT+Ai2ygdnm+yROfE0A,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA,
kdeus-hpIqsD4AKlfQT0dZR+AlfA, surenb-hpIqsD4AKlfQT0dZR+AlfA,
Samuel Ortiz, David S. Miller,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
According to ETSI TS 102 622 specification chapter 4.4 pipe identifier
is 7 bits long which allows for 128 unique pipe IDs. Because
NFC_HCI_MAX_PIPES is used as the number of pipes supported and not
as the max pipe ID, its value should be 128 instead of 127.
nfc_hci_recv_from_llc extracts pipe ID from packet header using
NFC_HCI_FRAGMENT(0x7F) mask which allows for pipe ID value of 127.
Same happens when NCI_HCP_MSG_GET_PIPE() is being used. With
pipes array having only 127 elements and pipe ID of 127 the OOB memory
access will result.
Suggested-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
include/net/nfc/hci.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/nfc/hci.h b/include/net/nfc/hci.h
index 316694dafa5b..008f466d1da7 100644
--- a/include/net/nfc/hci.h
+++ b/include/net/nfc/hci.h
@@ -87,7 +87,7 @@ struct nfc_hci_pipe {
* According to specification 102 622 chapter 4.4 Pipes,
* the pipe identifier is 7 bits long.
*/
-#define NFC_HCI_MAX_PIPES 127
+#define NFC_HCI_MAX_PIPES 128
struct nfc_hci_init_data {
u8 gate_count;
struct nfc_hci_gate gates[NFC_HCI_MAX_CUSTOM_GATES];
--
2.18.0.865.gffc8e1a3cd6-goog
^ permalink raw reply related
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands
From: Kees Cook @ 2018-08-14 21:49 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz,
David S. Miller, Allen Pais, linux-wireless, Network Development,
LKML
In-Reply-To: <CAJuCfpEX69Sbd=0i0ismP6q2NsZGUF4gMSCr1G5NdXd3n1NyZA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> Thanks. This is great. I'm so glad these are finally getting fixed.
>>>>>
>>>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
>>>>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
>>>>> (0x7f) so that's one element beyond the end of the array and the
>>>>> NFC_HCI_HCP_RESPONSE isn't checked.
>>>>>
>>>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
>>>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one.
>>>>
>>>> Good point. From hci.h:
>>>>
>>>> /*
>>>> * According to specification 102 622 chapter 4.4 Pipes,
>>>> * the pipe identifier is 7 bits long.
>>>> */
>>>> #define NFC_HCI_MAX_PIPES 127
>>>>
>>>> And then:
>>>>
>>>> struct nfc_hci_dev {
>>>> ...
>>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
>>>> ...
>>>> }
>>>>
>>>> I think the correct fix would be to change it to:
>>>>
>>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];
>>>>
>>>> What do you think?
>>>>
>>>
>>> Just to be clear, this would fix the problem Dan described in his
>>> reply and it should be implemented in a separate patch. The original
>>> fix is still valid.
>>
>> I think you could merge the fixes into a single patch.
>
> Couple reasons I would prefer to keep them separate:
> - I feel that descriptions for these two issues should be different
> and it's easier if we don't mix them up
> - This one is already merged into Android kernels, so would be easier
> to introduce the second fix separately
> - I would like to give credit to people who noticed the problem (in
> this case those are different people)
>
> However if more people think we should fix both issues in the same
> patch I'll happily do that.
> Thanks!
If it's already landed separately somewhere else, then yeah, 2 patches
sounds good. No objection either way from me!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH next-queue 3/8] ixgbe: add VF ipsec management
From: Shannon Nelson @ 2018-08-14 18:55 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, intel-wired-lan, jeffrey.t.kirsher, steffen.klassert,
netdev
In-Reply-To: <201808141311.4srvjTmi%fengguang.wu@intel.com>
On 8/13/2018 10:31 PM, kbuild test robot wrote:
> Hi Shannon,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on jkirsher-next-queue/dev-queue]
> [also build test ERROR on v4.18 next-20180813]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Shannon-Nelson/ixgbe-ixgbevf-IPsec-offload-support-for-VFs/20180814-074800
> base: https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue
> config: x86_64-randconfig-v0-08131550 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.o: In function `ixgbe_ipsec_vf_add_sa':
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c:917: undefined reference to `xfrm_aead_get_byname'
> make[1]: *** [vmlinux] Error 1
> make[1]: Target '_all' not remade because of errors.
>
Huh, odd. I'm not able to reproduce this error using your config file
in a net-next tree of vintage v4.18-rc8 or in Jeff's dev-queue branch.
It looks like I'm using an older compiler (4.8.5) but that shouldn't
make a difference here.
sln
^ permalink raw reply
* [PATCH net-next] net: sched: always disable bh when taking tcf_lock
From: Vlad Buslov @ 2018-08-14 18:46 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, Vlad Buslov
Recently, ops->init() and ops->dump() of all actions were modified to
always obtain tcf_lock when accessing private action state. Actions that
don't depend on tcf_lock for synchronization with their data path use
non-bh locking API. However, tcf_lock is also used to protect rate
estimator stats in softirq context by timer callback.
Change ops->init() and ops->dump() of all actions to disable bh when using
tcf_lock to prevent deadlock reported by following lockdep warning:
[ 105.470398] ================================
[ 105.475014] WARNING: inconsistent lock state
[ 105.479628] 4.18.0-rc8+ #664 Not tainted
[ 105.483897] --------------------------------
[ 105.488511] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
[ 105.494871] swapper/16/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
[ 105.500449] 00000000f86c012e (&(&p->tcfa_lock)->rlock){+.?.}, at: est_fetch_counters+0x3c/0xa0
[ 105.509696] {SOFTIRQ-ON-W} state was registered at:
[ 105.514925] _raw_spin_lock+0x2c/0x40
[ 105.519022] tcf_bpf_init+0x579/0x820 [act_bpf]
[ 105.523990] tcf_action_init_1+0x4e4/0x660
[ 105.528518] tcf_action_init+0x1ce/0x2d0
[ 105.532880] tcf_exts_validate+0x1d8/0x200
[ 105.537416] fl_change+0x55a/0x268b [cls_flower]
[ 105.542469] tc_new_tfilter+0x748/0xa20
[ 105.546738] rtnetlink_rcv_msg+0x56a/0x6d0
[ 105.551268] netlink_rcv_skb+0x18d/0x200
[ 105.555628] netlink_unicast+0x2d0/0x370
[ 105.559990] netlink_sendmsg+0x3b9/0x6a0
[ 105.564349] sock_sendmsg+0x6b/0x80
[ 105.568271] ___sys_sendmsg+0x4a1/0x520
[ 105.572547] __sys_sendmsg+0xd7/0x150
[ 105.576655] do_syscall_64+0x72/0x2c0
[ 105.580757] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 105.586243] irq event stamp: 489296
[ 105.590084] hardirqs last enabled at (489296): [<ffffffffb507e639>] _raw_spin_unlock_irq+0x29/0x40
[ 105.599765] hardirqs last disabled at (489295): [<ffffffffb507e745>] _raw_spin_lock_irq+0x15/0x50
[ 105.609277] softirqs last enabled at (489292): [<ffffffffb413a6a3>] irq_enter+0x83/0xa0
[ 105.618001] softirqs last disabled at (489293): [<ffffffffb413a800>] irq_exit+0x140/0x190
[ 105.626813]
other info that might help us debug this:
[ 105.633976] Possible unsafe locking scenario:
[ 105.640526] CPU0
[ 105.643325] ----
[ 105.646125] lock(&(&p->tcfa_lock)->rlock);
[ 105.650747] <Interrupt>
[ 105.653717] lock(&(&p->tcfa_lock)->rlock);
[ 105.658514]
*** DEADLOCK ***
[ 105.665349] 1 lock held by swapper/16/0:
[ 105.669629] #0: 00000000a640ad99 ((&est->timer)){+.-.}, at: call_timer_fn+0x10b/0x550
[ 105.678200]
stack backtrace:
[ 105.683194] CPU: 16 PID: 0 Comm: swapper/16 Not tainted 4.18.0-rc8+ #664
[ 105.690249] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 105.698626] Call Trace:
[ 105.701421] <IRQ>
[ 105.703791] dump_stack+0x92/0xeb
[ 105.707461] print_usage_bug+0x336/0x34c
[ 105.711744] mark_lock+0x7c9/0x980
[ 105.715500] ? print_shortest_lock_dependencies+0x2e0/0x2e0
[ 105.721424] ? check_usage_forwards+0x230/0x230
[ 105.726315] __lock_acquire+0x923/0x26f0
[ 105.730597] ? debug_show_all_locks+0x240/0x240
[ 105.735478] ? mark_lock+0x493/0x980
[ 105.739412] ? check_chain_key+0x140/0x1f0
[ 105.743861] ? __lock_acquire+0x836/0x26f0
[ 105.748323] ? lock_acquire+0x12e/0x290
[ 105.752516] lock_acquire+0x12e/0x290
[ 105.756539] ? est_fetch_counters+0x3c/0xa0
[ 105.761084] _raw_spin_lock+0x2c/0x40
[ 105.765099] ? est_fetch_counters+0x3c/0xa0
[ 105.769633] est_fetch_counters+0x3c/0xa0
[ 105.773995] est_timer+0x87/0x390
[ 105.777670] ? est_fetch_counters+0xa0/0xa0
[ 105.782210] ? lock_acquire+0x12e/0x290
[ 105.786410] call_timer_fn+0x161/0x550
[ 105.790512] ? est_fetch_counters+0xa0/0xa0
[ 105.795055] ? del_timer_sync+0xd0/0xd0
[ 105.799249] ? __lock_is_held+0x93/0x110
[ 105.803531] ? mark_held_locks+0x20/0xe0
[ 105.807813] ? _raw_spin_unlock_irq+0x29/0x40
[ 105.812525] ? est_fetch_counters+0xa0/0xa0
[ 105.817069] ? est_fetch_counters+0xa0/0xa0
[ 105.821610] run_timer_softirq+0x3c4/0x9f0
[ 105.826064] ? lock_acquire+0x12e/0x290
[ 105.830257] ? __bpf_trace_timer_class+0x10/0x10
[ 105.835237] ? __lock_is_held+0x25/0x110
[ 105.839517] __do_softirq+0x11d/0x7bf
[ 105.843542] irq_exit+0x140/0x190
[ 105.847208] smp_apic_timer_interrupt+0xac/0x3b0
[ 105.852182] apic_timer_interrupt+0xf/0x20
[ 105.856628] </IRQ>
[ 105.859081] RIP: 0010:cpuidle_enter_state+0xd8/0x4d0
[ 105.864395] Code: 46 ff 48 89 44 24 08 0f 1f 44 00 00 31 ff e8 cf ec 46 ff 80 7c 24 07 00 0f 85 1d 02 00 00 e8 9f 90 4b ff fb 66 0f 1f 44 00 00 <4c> 8b 6c 24 08 4d 29 fd 0f 80 36 03 00 00 4c 89 e8 48 ba cf f7 53
[ 105.884288] RSP: 0018:ffff8803ad94fd20 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
[ 105.892494] RAX: 0000000000000000 RBX: ffffe8fb300829c0 RCX: ffffffffb41e19e1
[ 105.899988] RDX: 0000000000000007 RSI: dffffc0000000000 RDI: ffff8803ad9358ac
[ 105.907503] RBP: ffffffffb6636300 R08: 0000000000000004 R09: 0000000000000000
[ 105.914997] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000004
[ 105.922487] R13: ffffffffb6636140 R14: ffffffffb66362d8 R15: 000000188d36091b
[ 105.929988] ? trace_hardirqs_on_caller+0x141/0x2d0
[ 105.935232] do_idle+0x28e/0x320
[ 105.938817] ? arch_cpu_idle_exit+0x40/0x40
[ 105.943361] ? mark_lock+0x8c1/0x980
[ 105.947295] ? _raw_spin_unlock_irqrestore+0x32/0x60
[ 105.952619] cpu_startup_entry+0xc2/0xd0
[ 105.956900] ? cpu_in_idle+0x20/0x20
[ 105.960830] ? _raw_spin_unlock_irqrestore+0x32/0x60
[ 105.966146] ? trace_hardirqs_on_caller+0x141/0x2d0
[ 105.971391] start_secondary+0x2b5/0x360
[ 105.975669] ? set_cpu_sibling_map+0x1330/0x1330
[ 105.980654] secondary_startup_64+0xa5/0xb0
Taking tcf_lock in sample action with bh disabled causes lockdep to issue a
warning regarding possible irq lock inversion dependency between tcf_lock,
and psample_groups_lock that is taken when holding tcf_lock in sample init:
[ 162.108959] Possible interrupt unsafe locking scenario:
[ 162.116386] CPU0 CPU1
[ 162.121277] ---- ----
[ 162.126162] lock(psample_groups_lock);
[ 162.130447] local_irq_disable();
[ 162.136772] lock(&(&p->tcfa_lock)->rlock);
[ 162.143957] lock(psample_groups_lock);
[ 162.150813] <Interrupt>
[ 162.153808] lock(&(&p->tcfa_lock)->rlock);
[ 162.158608]
*** DEADLOCK ***
In order to prevent potential lock inversion dependency between tcf_lock
and psample_groups_lock, extract call to psample_group_get() from tcf_lock
protected section in sample action init function.
Fixes: 4e232818bd32 ("net: sched: act_mirred: remove dependency on rtnl lock")
Fixes: 764e9a24480f ("net: sched: act_vlan: remove dependency on rtnl lock")
Fixes: 729e01260989 ("net: sched: act_tunnel_key: remove dependency on rtnl lock")
Fixes: d77284956656 ("net: sched: act_sample: remove dependency on rtnl lock")
Fixes: e8917f437006 ("net: sched: act_gact: remove dependency on rtnl lock")
Fixes: b6a2b971c0b0 ("net: sched: act_csum: remove dependency on rtnl lock")
Fixes: 2142236b4584 ("net: sched: act_bpf: remove dependency on rtnl lock")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_bpf.c | 10 +++++-----
net/sched/act_csum.c | 10 +++++-----
net/sched/act_gact.c | 10 +++++-----
net/sched/act_mirred.c | 16 ++++++++--------
net/sched/act_sample.c | 25 ++++++++++++++-----------
net/sched/act_tunnel_key.c | 10 +++++-----
net/sched/act_vlan.c | 10 +++++-----
7 files changed, 47 insertions(+), 44 deletions(-)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 9e8a33f9fee3..3d56ab69614c 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -147,7 +147,7 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act,
struct tcf_t tm;
int ret;
- spin_lock(&prog->tcf_lock);
+ spin_lock_bh(&prog->tcf_lock);
opt.action = prog->tcf_action;
if (nla_put(skb, TCA_ACT_BPF_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -164,11 +164,11 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act,
TCA_ACT_BPF_PAD))
goto nla_put_failure;
- spin_unlock(&prog->tcf_lock);
+ spin_unlock_bh(&prog->tcf_lock);
return skb->len;
nla_put_failure:
- spin_unlock(&prog->tcf_lock);
+ spin_unlock_bh(&prog->tcf_lock);
nlmsg_trim(skb, tp);
return -1;
}
@@ -340,7 +340,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
prog = to_bpf(*act);
- spin_lock(&prog->tcf_lock);
+ spin_lock_bh(&prog->tcf_lock);
if (res != ACT_P_CREATED)
tcf_bpf_prog_fill_cfg(prog, &old);
@@ -352,7 +352,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
prog->tcf_action = parm->action;
rcu_assign_pointer(prog->filter, cfg.filter);
- spin_unlock(&prog->tcf_lock);
+ spin_unlock_bh(&prog->tcf_lock);
if (res == ACT_P_CREATED) {
tcf_idr_insert(tn, *act);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index f01c59ba6d12..422fdcd9b196 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -96,11 +96,11 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
}
params_new->update_flags = parm->update_flags;
- spin_lock(&p->tcf_lock);
+ spin_lock_bh(&p->tcf_lock);
p->tcf_action = parm->action;
rcu_swap_protected(p->params, params_new,
lockdep_is_held(&p->tcf_lock));
- spin_unlock(&p->tcf_lock);
+ spin_unlock_bh(&p->tcf_lock);
if (params_new)
kfree_rcu(params_new, rcu);
@@ -604,7 +604,7 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
};
struct tcf_t t;
- spin_lock(&p->tcf_lock);
+ spin_lock_bh(&p->tcf_lock);
params = rcu_dereference_protected(p->params,
lockdep_is_held(&p->tcf_lock));
opt.action = p->tcf_action;
@@ -616,12 +616,12 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
tcf_tm_dump(&t, &p->tcf_tm);
if (nla_put_64bit(skb, TCA_CSUM_TM, sizeof(t), &t, TCA_CSUM_PAD))
goto nla_put_failure;
- spin_unlock(&p->tcf_lock);
+ spin_unlock_bh(&p->tcf_lock);
return skb->len;
nla_put_failure:
- spin_unlock(&p->tcf_lock);
+ spin_unlock_bh(&p->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index bfccd34a3968..e5fd620c034a 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -113,7 +113,7 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
gact = to_gact(*a);
- spin_lock(&gact->tcf_lock);
+ spin_lock_bh(&gact->tcf_lock);
gact->tcf_action = parm->action;
#ifdef CONFIG_GACT_PROB
if (p_parm) {
@@ -126,7 +126,7 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
gact->tcfg_ptype = p_parm->ptype;
}
#endif
- spin_unlock(&gact->tcf_lock);
+ spin_unlock_bh(&gact->tcf_lock);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -183,7 +183,7 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
};
struct tcf_t t;
- spin_lock(&gact->tcf_lock);
+ spin_lock_bh(&gact->tcf_lock);
opt.action = gact->tcf_action;
if (nla_put(skb, TCA_GACT_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -202,12 +202,12 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &gact->tcf_tm);
if (nla_put_64bit(skb, TCA_GACT_TM, sizeof(t), &t, TCA_GACT_PAD))
goto nla_put_failure;
- spin_unlock(&gact->tcf_lock);
+ spin_unlock_bh(&gact->tcf_lock);
return skb->len;
nla_put_failure:
- spin_unlock(&gact->tcf_lock);
+ spin_unlock_bh(&gact->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 327be257033d..93138c61f8d1 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -159,14 +159,14 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
}
m = to_mirred(*a);
- spin_lock(&m->tcf_lock);
+ spin_lock_bh(&m->tcf_lock);
m->tcf_action = parm->action;
m->tcfm_eaction = parm->eaction;
if (parm->ifindex) {
dev = dev_get_by_index(net, parm->ifindex);
if (!dev) {
- spin_unlock(&m->tcf_lock);
+ spin_unlock_bh(&m->tcf_lock);
tcf_idr_release(*a, bind);
return -ENODEV;
}
@@ -177,7 +177,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
dev_put(dev);
m->tcfm_mac_header_xmit = mac_header_xmit;
}
- spin_unlock(&m->tcf_lock);
+ spin_unlock_bh(&m->tcf_lock);
if (ret == ACT_P_CREATED) {
spin_lock(&mirred_list_lock);
@@ -305,7 +305,7 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
struct net_device *dev;
struct tcf_t t;
- spin_lock(&m->tcf_lock);
+ spin_lock_bh(&m->tcf_lock);
opt.action = m->tcf_action;
opt.eaction = m->tcfm_eaction;
dev = tcf_mirred_dev_dereference(m);
@@ -318,12 +318,12 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
tcf_tm_dump(&t, &m->tcf_tm);
if (nla_put_64bit(skb, TCA_MIRRED_TM, sizeof(t), &t, TCA_MIRRED_PAD))
goto nla_put_failure;
- spin_unlock(&m->tcf_lock);
+ spin_unlock_bh(&m->tcf_lock);
return skb->len;
nla_put_failure:
- spin_unlock(&m->tcf_lock);
+ spin_unlock_bh(&m->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
@@ -356,7 +356,7 @@ static int mirred_device_event(struct notifier_block *unused,
if (event == NETDEV_UNREGISTER) {
spin_lock(&mirred_list_lock);
list_for_each_entry(m, &mirred_list, tcfm_list) {
- spin_lock(&m->tcf_lock);
+ spin_lock_bh(&m->tcf_lock);
if (tcf_mirred_dev_dereference(m) == dev) {
dev_put(dev);
/* Note : no rcu grace period necessary, as
@@ -364,7 +364,7 @@ static int mirred_device_event(struct notifier_block *unused,
*/
RCU_INIT_POINTER(m->tcfm_dev, NULL);
}
- spin_unlock(&m->tcf_lock);
+ spin_unlock_bh(&m->tcf_lock);
}
spin_unlock(&mirred_list_lock);
}
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 81071afe1b43..207b4132d1b0 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -44,6 +44,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
struct nlattr *tb[TCA_SAMPLE_MAX + 1];
struct psample_group *psample_group;
struct tc_sample *parm;
+ u32 psample_group_num;
struct tcf_sample *s;
bool exists = false;
int ret, err;
@@ -78,25 +79,27 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
tcf_idr_release(*a, bind);
return -EEXIST;
}
- s = to_sample(*a);
- spin_lock(&s->tcf_lock);
- s->tcf_action = parm->action;
- s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
- s->psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
- psample_group = psample_group_get(net, s->psample_group_num);
+ psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
+ psample_group = psample_group_get(net, psample_group_num);
if (!psample_group) {
- spin_unlock(&s->tcf_lock);
tcf_idr_release(*a, bind);
return -ENOMEM;
}
+
+ s = to_sample(*a);
+
+ spin_lock_bh(&s->tcf_lock);
+ s->tcf_action = parm->action;
+ s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
+ s->psample_group_num = psample_group_num;
RCU_INIT_POINTER(s->psample_group, psample_group);
if (tb[TCA_SAMPLE_TRUNC_SIZE]) {
s->truncate = true;
s->trunc_size = nla_get_u32(tb[TCA_SAMPLE_TRUNC_SIZE]);
}
- spin_unlock(&s->tcf_lock);
+ spin_unlock_bh(&s->tcf_lock);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -183,7 +186,7 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a,
};
struct tcf_t t;
- spin_lock(&s->tcf_lock);
+ spin_lock_bh(&s->tcf_lock);
opt.action = s->tcf_action;
if (nla_put(skb, TCA_SAMPLE_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -201,12 +204,12 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_u32(skb, TCA_SAMPLE_PSAMPLE_GROUP, s->psample_group_num))
goto nla_put_failure;
- spin_unlock(&s->tcf_lock);
+ spin_unlock_bh(&s->tcf_lock);
return skb->len;
nla_put_failure:
- spin_unlock(&s->tcf_lock);
+ spin_unlock_bh(&s->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index ba2ae9f75ef5..8f09cf08d8fe 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -354,11 +354,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
params_new->tcft_action = parm->t_action;
params_new->tcft_enc_metadata = metadata;
- spin_lock(&t->tcf_lock);
+ spin_lock_bh(&t->tcf_lock);
t->tcf_action = parm->action;
rcu_swap_protected(t->params, params_new,
lockdep_is_held(&t->tcf_lock));
- spin_unlock(&t->tcf_lock);
+ spin_unlock_bh(&t->tcf_lock);
if (params_new)
kfree_rcu(params_new, rcu);
@@ -485,7 +485,7 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
};
struct tcf_t tm;
- spin_lock(&t->tcf_lock);
+ spin_lock_bh(&t->tcf_lock);
params = rcu_dereference_protected(t->params,
lockdep_is_held(&t->tcf_lock));
opt.action = t->tcf_action;
@@ -520,12 +520,12 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_64bit(skb, TCA_TUNNEL_KEY_TM, sizeof(tm),
&tm, TCA_TUNNEL_KEY_PAD))
goto nla_put_failure;
- spin_unlock(&t->tcf_lock);
+ spin_unlock_bh(&t->tcf_lock);
return skb->len;
nla_put_failure:
- spin_unlock(&t->tcf_lock);
+ spin_unlock_bh(&t->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 5bde17fe3608..1470488a3fd0 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -213,10 +213,10 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
p->tcfv_push_prio = push_prio;
p->tcfv_push_proto = push_proto;
- spin_lock(&v->tcf_lock);
+ spin_lock_bh(&v->tcf_lock);
v->tcf_action = parm->action;
rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
- spin_unlock(&v->tcf_lock);
+ spin_unlock_bh(&v->tcf_lock);
if (p)
kfree_rcu(p, rcu);
@@ -249,7 +249,7 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
};
struct tcf_t t;
- spin_lock(&v->tcf_lock);
+ spin_lock_bh(&v->tcf_lock);
opt.action = v->tcf_action;
p = rcu_dereference_protected(v->vlan_p, lockdep_is_held(&v->tcf_lock));
opt.v_action = p->tcfv_action;
@@ -268,12 +268,12 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &v->tcf_tm);
if (nla_put_64bit(skb, TCA_VLAN_TM, sizeof(t), &t, TCA_VLAN_PAD))
goto nla_put_failure;
- spin_unlock(&v->tcf_lock);
+ spin_unlock_bh(&v->tcf_lock);
return skb->len;
nla_put_failure:
- spin_unlock(&v->tcf_lock);
+ spin_unlock_bh(&v->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related
* [PATCH] bluetooth/{bnep,cmtp,hidp}: Remove unnecessary smp_mb__{before,after}_atomic
From: Andrea Parri @ 2018-08-14 18:41 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, David S . Miller
Cc: linux-bluetooth, netdev, linux-kernel, Jeffy Chen, Brian Norris,
AL Yu-Chen Cho, Andrea Parri
The barriers are unneeded; wait_woken() and woken_wake_function()
already provide us with the required synchronization: remove them
and document that we're relying on the (implicit) synchronization
provided by wait_woken() and woken_wake_function().
Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
---
net/bluetooth/bnep/core.c | 7 ++++---
net/bluetooth/cmtp/core.c | 14 ++++++++------
net/bluetooth/hidp/core.c | 13 ++++++++-----
3 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index 7b3965861013c..43c284158f63e 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -489,9 +489,6 @@ static int bnep_session(void *arg)
add_wait_queue(sk_sleep(sk), &wait);
while (1) {
- /* Ensure session->terminate is updated */
- smp_mb__before_atomic();
-
if (atomic_read(&s->terminate))
break;
/* RX */
@@ -512,6 +509,10 @@ static int bnep_session(void *arg)
break;
netif_wake_queue(dev);
+ /*
+ * wait_woken() performs the necessary memory barriers
+ * for us; see the header comment for this primitive.
+ */
wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
remove_wait_queue(sk_sleep(sk), &wait);
diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 7f26a5a19ff6d..07cfa3249f83a 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -288,9 +288,6 @@ static int cmtp_session(void *arg)
add_wait_queue(sk_sleep(sk), &wait);
while (1) {
- /* Ensure session->terminate is updated */
- smp_mb__before_atomic();
-
if (atomic_read(&session->terminate))
break;
if (sk->sk_state != BT_CONNECTED)
@@ -306,6 +303,10 @@ static int cmtp_session(void *arg)
cmtp_process_transmit(session);
+ /*
+ * wait_woken() performs the necessary memory barriers
+ * for us; see the header comment for this primitive.
+ */
wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
remove_wait_queue(sk_sleep(sk), &wait);
@@ -431,9 +432,10 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
/* Stop session thread */
atomic_inc(&session->terminate);
- /* Ensure session->terminate is updated */
- smp_mb__after_atomic();
-
+ /*
+ * See the comment preceding the call to wait_woken()
+ * in cmtp_session().
+ */
wake_up_interruptible(sk_sleep(session->sock->sk));
} else
err = -ENOENT;
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 1036e4fa1ea28..e621551712f59 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1074,6 +1074,10 @@ static int hidp_session_start_sync(struct hidp_session *session)
static void hidp_session_terminate(struct hidp_session *session)
{
atomic_inc(&session->terminate);
+ /*
+ * See the comment preceding the call to wait_woken()
+ * in hidp_session_run().
+ */
wake_up_interruptible(&hidp_session_wq);
}
@@ -1193,8 +1197,6 @@ static void hidp_session_run(struct hidp_session *session)
* thread is woken up by ->sk_state_changed().
*/
- /* Ensure session->terminate is updated */
- smp_mb__before_atomic();
if (atomic_read(&session->terminate))
break;
@@ -1228,14 +1230,15 @@ static void hidp_session_run(struct hidp_session *session)
hidp_process_transmit(session, &session->ctrl_transmit,
session->ctrl_sock);
+ /*
+ * wait_woken() performs the necessary memory barriers
+ * for us; see the header comment for this primitive.
+ */
wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
remove_wait_queue(&hidp_session_wq, &wait);
atomic_inc(&session->terminate);
-
- /* Ensure session->terminate is updated */
- smp_mb__after_atomic();
}
static int hidp_session_wake_function(wait_queue_entry_t *wait,
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v1 2/3] zinc: Introduce minimal cryptography library
From: Eric Biggers @ 2018-08-14 21:12 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Eric Biggers, Linux Crypto Mailing List, LKML, Netdev,
David Miller, Andrew Lutomirski, Greg Kroah-Hartman, Samuel Neves,
Daniel J . Bernstein, Tanja Lange, Jean-Philippe Aumasson,
Karthikeyan Bhargavan
In-Reply-To: <CAHmME9pbwj0RaX4YM9epGOSgDRNUt=GA4UzO_iKcW=Md7kG6JQ@mail.gmail.com>
On Fri, Aug 03, 2018 at 04:33:50AM +0200, Jason A. Donenfeld wrote:
>
> > Also, earlier when I tested OpenSSL's ChaCha NEON implementation on ARM
> > Cortex-A7 it was actually quite a bit slower than the one in the Linux
> > kernel written by Ard Biesheuvel... I trust that when claiming the
> > performance of all implementations you're adding is "state-of-the-art
> > and unrivaled", you actually compared them to the ones already in the
> > Linux kernel which you're advocating replacing, right? :-)
>
> Yes, I have, and my results don't corroborate your findings. It will
> be interesting to get out a wider variety of hardware for comparisons.
> I suspect, also, that if the snarky emoticons subside, AndyP would be
> very interested in whatever we find and could have interest in
> improving implementations, should we actually find performance
> differences.
>
On ARM Cortex-A7, OpenSSL's ChaCha20 implementation is 13.9 cpb (cycles per
byte), whereas Linux's is faster: 11.9 cpb. I've also recently improved the
Linux implementation to 11.3 cpb and would like to send out a patch soon...
I've also written a scalar ChaCha20 implementation (no NEON instructions!) that
is 12.2 cpb on one block at a time on Cortex-A7, taking advantage of the free
rotates; that would be useful for the single permutation used to compute
XChaCha's subkey, and also for the ends of messages.
The reason Linux's ChaCha20 NEON implementation is faster than OpenSSL's is that
Linux's does 4 blocks at once using NEON instructions, and the words are
de-interleaved so the rows don't need to be shifted between each round.
OpenSSL's implementation, on the other hand, only does 3 blocks at once with
NEON instructions and has to shift the rows between each round. OpenSSL's
implementation also does a 4th block at the same time using regular ARM
instructions, but that doesn't help on Cortex-A7; it just makes it slower.
I understand there are tradeoffs, and different implementations can be faster on
different CPUs. Just know that from my point of view, switching to the OpenSSL
implementation actually introduces a performance regression, and we care a *lot*
about this since we need ChaCha to be absolutely as fast as possible for HPolyC
disk encryption. So if your proposal goes in, I'd likely need to write a patch
to get the old performance back, at least on Cortex-A7...
Also, I don't know whether Andy P. considered the 4xNEON implementation
technique. It could even be fastest on other ARM CPUs too, I don't know.
- Eric
^ permalink raw reply
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands
From: Suren Baghdasaryan @ 2018-08-14 20:55 UTC (permalink / raw)
To: Kees Cook
Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz,
David S. Miller, Allen Pais, linux-wireless, Network Development,
LKML
In-Reply-To: <CAGXu5jKqcpbpG0Ky4-yXV4d5q10HtGwJtdyPKcrwkJUPhA_Mpg@mail.gmail.com>
On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote:
>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote:
>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>> Thanks. This is great. I'm so glad these are finally getting fixed.
>>>>
>>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
>>>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
>>>> (0x7f) so that's one element beyond the end of the array and the
>>>> NFC_HCI_HCP_RESPONSE isn't checked.
>>>>
>>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
>>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one.
>>>
>>> Good point. From hci.h:
>>>
>>> /*
>>> * According to specification 102 622 chapter 4.4 Pipes,
>>> * the pipe identifier is 7 bits long.
>>> */
>>> #define NFC_HCI_MAX_PIPES 127
>>>
>>> And then:
>>>
>>> struct nfc_hci_dev {
>>> ...
>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
>>> ...
>>> }
>>>
>>> I think the correct fix would be to change it to:
>>>
>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];
>>>
>>> What do you think?
>>>
>>
>> Just to be clear, this would fix the problem Dan described in his
>> reply and it should be implemented in a separate patch. The original
>> fix is still valid.
>
> I think you could merge the fixes into a single patch.
Couple reasons I would prefer to keep them separate:
- I feel that descriptions for these two issues should be different
and it's easier if we don't mix them up
- This one is already merged into Android kernels, so would be easier
to introduce the second fix separately
- I would like to give credit to people who noticed the problem (in
this case those are different people)
However if more people think we should fix both issues in the same
patch I'll happily do that.
Thanks!
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
^ permalink raw reply
* [PATCH bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core()
From: Yonghong Song @ 2018-08-14 18:01 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: kernel-team, Roman Gushchin
Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers
to the cgroup storage") refactored the bpf_prog_array_copy_core()
to accommodate new structure bpf_prog_array_item which contains
bpf_prog array itself.
In the old code, we had
perf_event_query_prog_array():
mutex_lock(...)
bpf_prog_array_copy_call():
prog = rcu_dereference_check(array, 1)->progs
bpf_prog_array_copy_core(prog, ...)
mutex_unlock(...)
With the above commit, we had
perf_event_query_prog_array():
mutex_lock(...)
bpf_prog_array_copy_call():
bpf_prog_array_copy_core(array, ...):
item = rcu_dereference(array)->items;
...
mutex_unlock(...)
The new code will trigger a lockdep rcu checking warning.
The fix is to change rcu_dereference() to rcu_dereference_check()
to prevent such a warning.
Reported-by: syzbot+6e72317008eef84a216b@syzkaller.appspotmail.com
Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage")
Cc: Roman Gushchin <guro@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
kernel/bpf/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 4d09e610777f..3f5bf1af0826 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1579,7 +1579,7 @@ static bool bpf_prog_array_copy_core(struct bpf_prog_array __rcu *array,
struct bpf_prog_array_item *item;
int i = 0;
- item = rcu_dereference(array)->items;
+ item = rcu_dereference_check(array, 1)->items;
for (; item->prog; item++) {
if (item->prog == &dummy_bpf_prog.prog)
continue;
--
2.17.1
^ permalink raw reply related
* RE: [PATCH] dt-bindings: net: ravb: Add support for r8a774a1 SoC
From: Fabrizio Castro @ 2018-08-14 17:58 UTC (permalink / raw)
To: David Miller
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
sergei.shtylyov@cogentembedded.com, geert+renesas@glider.be,
horms+renesas@verge.net.au, Biju Das, Yoshihiro Shimoda,
netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
horms@verge.net.au, Chris Paterson
In-Reply-To: <20180814.101934.1621967443721994871.davem@davemloft.net>
Hello David,
> From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Date: Tue, 14 Aug 2018 17:13:30 +0000
>
> > Hello David,
> >
> >> Subject: Re: [PATCH] dt-bindings: net: ravb: Add support for r8a774a1 SoC
> >>
> >> From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >> Date: Tue, 14 Aug 2018 13:33:37 +0100
> >>
> >> > Document RZ/G2M (R8A774A1) SoC bindings.
> >> >
> >> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >> > Reviewed-by: Biju Das <biju.das@bp.renesas.com>
> >>
> >> Which tree is this targetting?
> >
> > This patch applies on next-20180814
>
> I'm asking something different. I'm asking which subsystem tree, and
> which maintainer, should apply this patch.
I think this is appropriate for net-next, and as far as I can tell previous patches have either been applied by you (mostly) or Rob Herring.
Maybe I should have put "[PATCH net-next]" in the Subject like the following?
https://patchwork.kernel.org/patch/10314835/
Thanks,
Fab
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
^ permalink raw reply
* Re: [PATCH 1/3] dt-bindings: net: dsa: Add compatibility strings for Broadcom Omega
From: Rob Herring @ 2018-08-14 20:41 UTC (permalink / raw)
To: Arun Parameswaran
Cc: David S. Miller, Florian Fainelli, Andrew Lunn, Vivien Didelot,
Mark Rutland, netdev, devicetree, linux-kernel,
bcm-kernel-feedback-list, Arun Parameswaran
In-Reply-To: <1533661364-15381-2-git-send-email-arun.parameswaran@broadcom.com>
On Tue, 7 Aug 2018 10:02:42 -0700, Arun Parameswaran wrote:
> Add compatibility strings for the internal switch in the Broadcom
> Omega SoC family (BCM5831X/BCM1140X) to B53.
>
> Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
> ---
> Documentation/devicetree/bindings/net/dsa/b53.txt | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] hv/netvsc: Fix NULL dereference at single queue mode fallback
From: Takashi Iwai @ 2018-08-14 17:47 UTC (permalink / raw)
To: David Miller; +Cc: sthemmin, kys, haiyangz, devel, netdev
In-Reply-To: <20180814.102932.1425242139845133705.davem@davemloft.net>
On Tue, 14 Aug 2018 19:29:32 +0200,
David Miller wrote:
>
> From: Takashi Iwai <tiwai@suse.de>
> Date: Tue, 14 Aug 2018 19:10:50 +0200
>
> > The recent commit 916c5e1413be ("hv/netvsc: fix handling of fallback
> > to single queue mode") tried to fix the fallback behavior to a single
> > queue mode, but it changed the function to return zero incorrectly,
> > while the function should return an object pointer. Eventually this
> > leads to a NULL dereference at the callers that expect non-NULL
> > value.
> >
> > Fix it by returning the proper net_device object.
> >
> > Fixes: 916c5e1413be ("hv/netvsc: fix handling of fallback to single queue mode")
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
>
> Applied and queued up for -stable.
>
> Please do not put explicit "CC: stable" notations in networking patches, I queue
> up and submit networking patches to -stable explicitly.
OK, noted for the next time. Thanks!
Takashi
^ permalink raw reply
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands
From: Kees Cook @ 2018-08-14 20:33 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz,
David S. Miller, Allen Pais, linux-wireless, Network Development,
LKML
In-Reply-To: <CAJuCfpHSZuabLr6+t_n7UDamJCAP6EPTNEZ6kNqWOhk2mpYADg@mail.gmail.com>
On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote:
> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote:
>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>> Thanks. This is great. I'm so glad these are finally getting fixed.
>>>
>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
>>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
>>> (0x7f) so that's one element beyond the end of the array and the
>>> NFC_HCI_HCP_RESPONSE isn't checked.
>>>
>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one.
>>
>> Good point. From hci.h:
>>
>> /*
>> * According to specification 102 622 chapter 4.4 Pipes,
>> * the pipe identifier is 7 bits long.
>> */
>> #define NFC_HCI_MAX_PIPES 127
>>
>> And then:
>>
>> struct nfc_hci_dev {
>> ...
>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
>> ...
>> }
>>
>> I think the correct fix would be to change it to:
>>
>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];
>>
>> What do you think?
>>
>
> Just to be clear, this would fix the problem Dan described in his
> reply and it should be implemented in a separate patch. The original
> fix is still valid.
I think you could merge the fixes into a single patch.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH][bpf-next] bpf: test: fix spelling mistake "REUSEEPORT" -> "REUSEPORT"
From: David Miller @ 2018-08-14 17:43 UTC (permalink / raw)
To: alexei.starovoitov; +Cc: colin.king, ast, daniel, shuah, netdev
In-Reply-To: <20180814173911.ymxmgb3vehfu4wfd@ast-mbp>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 14 Aug 2018 10:39:12 -0700
> On Mon, Aug 13, 2018 at 03:00:32PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Trivial fix to spelling mistake in error message
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
>
> Dave, may be you can take this one directly, since PR for net-next is not yet sent to Linus?
Sure, done.
^ permalink raw reply
* Re: [PATCH][bpf-next] bpf: test: fix spelling mistake "REUSEEPORT" -> "REUSEPORT"
From: Alexei Starovoitov @ 2018-08-14 17:39 UTC (permalink / raw)
To: Colin King; +Cc: Alexei Starovoitov, Daniel Borkmann, Shuah Khan, netdev, davem
In-Reply-To: <20180813140032.2208-1-colin.king@canonical.com>
On Mon, Aug 13, 2018 at 03:00:32PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Trivial fix to spelling mistake in error message
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Dave, may be you can take this one directly, since PR for net-next is not yet sent to Linus?
^ permalink raw reply
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands
From: Suren Baghdasaryan @ 2018-08-14 20:26 UTC (permalink / raw)
To: Dan Carpenter
Cc: security, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais,
Kees Cook, linux-wireless, netdev, linux-kernel
In-Reply-To: <CAJuCfpF0Y6wc1z-rbn2wdWROgc_FkD2EX5HzDMcZ1oB5Z=dXSw@mail.gmail.com>
On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote:
> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> Thanks. This is great. I'm so glad these are finally getting fixed.
>>
>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
>> (0x7f) so that's one element beyond the end of the array and the
>> NFC_HCI_HCP_RESPONSE isn't checked.
>>
>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
>> NCI_HCP_MSG_GET_PIPE() so those could be off by one.
>
> Good point. From hci.h:
>
> /*
> * According to specification 102 622 chapter 4.4 Pipes,
> * the pipe identifier is 7 bits long.
> */
> #define NFC_HCI_MAX_PIPES 127
>
> And then:
>
> struct nfc_hci_dev {
> ...
> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
> ...
> }
>
> I think the correct fix would be to change it to:
>
> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];
>
> What do you think?
>
Just to be clear, this would fix the problem Dan described in his
reply and it should be implemented in a separate patch. The original
fix is still valid.
>>
>> regards,
>> dan carpenter
>>
^ permalink raw reply
* Re: [PATCH net-next] net: sched: act_ife: disable bh when taking ife_mod_lock
From: Vlad Buslov @ 2018-08-14 17:34 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jiri Pirko
In-Reply-To: <CAM_iQpUk6vo_9StceKQ+RLgB6FN_+yxPGC6BSeKsKwpWjGdw7Q@mail.gmail.com>
On Mon 13 Aug 2018 at 23:18, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Hi, Vlad,
>
> Could you help to test my fixes?
>
> I just pushed them into my own git repo:
> https://github.com/congwang/linux/commits/net-sched-fixes
>
> Particularly, this is the revert:
> https://github.com/congwang/linux/commit/b3f51c4ab8272cc8d3244848e528fce1426c4659
> and this is my fix for the lockdep warning you reported:
> https://github.com/congwang/linux/commit/ecadcde94919183e9f0d5bc376f05e731baf2661
>
> I don't have environment to test ife modules.
Hi Cong,
I've run the test with your patch applied and couldn't reproduce the
lockdep warning.
>
> BTW, this is the fix for the deadlock I spotted:
> https://github.com/congwang/linux/commit/44f3d7f5b6ed2d4a46177e6c658fa23b76141afa
>
> Thanks!
^ permalink raw reply
* [PATCH net-next] net: sched: act_ife: always release ife action on init error
From: Vlad Buslov @ 2018-08-14 17:29 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, jiri, Vlad Buslov
Action init API was changed to always take reference to action, even when
overwriting existing action. Substitute conditional action release, which
was executed only if action is newly created, with unconditional release in
tcf_ife_init() error handling code to prevent double free or memory leak in
case of overwrite.
Fixes: 4e8ddd7f1758 ("net: sched: don't release reference on action overwrite")
Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_ife.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 5d200495e467..c524edcad900 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -551,9 +551,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
NULL, NULL);
if (err) {
metadata_parse_err:
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
-
if (exists)
spin_unlock_bh(&ife->tcf_lock);
tcf_idr_release(*a, bind);
@@ -574,11 +571,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
*/
err = use_all_metadata(ife);
if (err) {
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
-
if (exists)
spin_unlock_bh(&ife->tcf_lock);
+ tcf_idr_release(*a, bind);
+
kfree(p);
return err;
}
--
2.7.5
^ permalink raw reply related
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