* Re: [PATCH net-next] rds: avoid lock hierarchy violation between m_rs_lock and rs_recv_lock
From: Santosh Shilimkar @ 2018-08-08 22:37 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: netdev, davem, rds-devel
In-Reply-To: <20180808221811.GA16895@oracle.com>
On 8/8/2018 3:18 PM, Sowmini Varadhan wrote:
> On (08/08/18 14:51), Santosh Shilimkar wrote:
>> This bug doesn't make sense since two different transports are using
>> same socket (Loop and rds_tcp) and running together.
>> For same transport, such race can't happen with MSG_ON_SOCK flag.
>> CPU1-> rds_loop_inc_free
>> CPU0 -> rds_tcp_cork ...
>>
>
> The test is just reporting a lock hierarchy violation
>
> As far as I can tell, this wasn't an actual deadlock that happened
> because as you point out, either a socket has the rds_tcp transport
> or the rds_loop transport, so this particular pair of stack traces
> would not happen with the code as it exists today.
>
Exactly.
> but there is a valid lock hierachy violation here, and
> imho it's a good idea to get that cleaned up.
>
The lock hierarchy violation is protected for the same transport.
I don't see this violation possible for legitimate use and hence
the comment. If we start supporting two different transport on
same socket then we have many more cases to fix and as such lock
violation will be just one of those.
Loop transport seems to keep throwing surprises. Need to
confirm but looks like it can co-exist with another transport
on same socket if those traces to be believed. If its the case,
then definitely that need to be plugged.
Regards,
Santosh
^ permalink raw reply
* [PATCH net 2/2] net/mlx5e: Cleanup of dcbnl related fields
From: Saeed Mahameed @ 2018-08-08 22:48 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Huy Nguyen, Yuval Shaia, Saeed Mahameed
In-Reply-To: <20180808224808.12600-1-saeedm@mellanox.com>
From: Huy Nguyen <huyn@mellanox.com>
Remove unused netdev_registered_init/remove in en.h
Return ENOSUPPORT if the check MLX5_DSCP_SUPPORTED fails.
Remove extra white space
Fixes: 2a5e7a1344f4 ("net/mlx5e: Add dcbnl dscp to priority support")
Signed-off-by: Huy Nguyen <huyn@mellanox.com>
Cc: Yuval Shaia <yuval.shaia@oracle.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 --
.../ethernet/mellanox/mlx5/core/en_dcbnl.c | 30 +++++++------------
2 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index eb9eb7aa953a..405236cf0b04 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -858,8 +858,6 @@ struct mlx5e_profile {
mlx5e_fp_handle_rx_cqe handle_rx_cqe;
mlx5e_fp_handle_rx_cqe handle_rx_cqe_mpwqe;
} rx_handlers;
- void (*netdev_registered_init)(struct mlx5e_priv *priv);
- void (*netdev_registered_remove)(struct mlx5e_priv *priv);
int max_tc;
};
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
index e33afa8d2417..722998d68564 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
@@ -443,16 +443,12 @@ static int mlx5e_dcbnl_ieee_setapp(struct net_device *dev, struct dcb_app *app)
bool is_new;
int err;
- if (app->selector != IEEE_8021QAZ_APP_SEL_DSCP)
- return -EINVAL;
-
- if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager))
- return -EINVAL;
-
- if (!MLX5_DSCP_SUPPORTED(priv->mdev))
- return -EINVAL;
+ if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager) ||
+ !MLX5_DSCP_SUPPORTED(priv->mdev))
+ return -EOPNOTSUPP;
- if (app->protocol >= MLX5E_MAX_DSCP)
+ if ((app->selector != IEEE_8021QAZ_APP_SEL_DSCP) ||
+ (app->protocol >= MLX5E_MAX_DSCP))
return -EINVAL;
/* Save the old entry info */
@@ -500,16 +496,12 @@ static int mlx5e_dcbnl_ieee_delapp(struct net_device *dev, struct dcb_app *app)
struct mlx5e_priv *priv = netdev_priv(dev);
int err;
- if (app->selector != IEEE_8021QAZ_APP_SEL_DSCP)
- return -EINVAL;
-
- if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager))
- return -EINVAL;
-
- if (!MLX5_DSCP_SUPPORTED(priv->mdev))
- return -EINVAL;
+ if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager) ||
+ !MLX5_DSCP_SUPPORTED(priv->mdev))
+ return -EOPNOTSUPP;
- if (app->protocol >= MLX5E_MAX_DSCP)
+ if ((app->selector != IEEE_8021QAZ_APP_SEL_DSCP) ||
+ (app->protocol >= MLX5E_MAX_DSCP))
return -EINVAL;
/* Skip if no dscp app entry */
@@ -1146,7 +1138,7 @@ static int mlx5e_set_trust_state(struct mlx5e_priv *priv, u8 trust_state)
{
int err;
- err = mlx5_set_trust_state(priv->mdev, trust_state);
+ err = mlx5_set_trust_state(priv->mdev, trust_state);
if (err)
return err;
priv->dcbx_dp.trust_state = trust_state;
--
2.17.0
^ permalink raw reply related
* [PATCH net 1/2] net/mlx5e: Properly check if hairpin is possible between two functions
From: Saeed Mahameed @ 2018-08-08 22:48 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Or Gerlitz, Saeed Mahameed
In-Reply-To: <20180808224808.12600-1-saeedm@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
The current check relies on function BDF addresses and can get
us wrong e.g when two VFs are assigned into a VM and the PCI
v-address is set by the hypervisor.
Fixes: 5c65c564c962 ('net/mlx5e: Support offloading TC NIC hairpin flows')
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
Reported-by: Alaa Hleihel <alaa@mellanox.com>
Tested-by: Alaa Hleihel <alaa@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 3a2c4e548226..dfbcda0d0e08 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1970,15 +1970,15 @@ static bool actions_match_supported(struct mlx5e_priv *priv,
static bool same_hw_devs(struct mlx5e_priv *priv, struct mlx5e_priv *peer_priv)
{
struct mlx5_core_dev *fmdev, *pmdev;
- u16 func_id, peer_id;
+ u64 fsystem_guid, psystem_guid;
fmdev = priv->mdev;
pmdev = peer_priv->mdev;
- func_id = (u16)((fmdev->pdev->bus->number << 8) | PCI_SLOT(fmdev->pdev->devfn));
- peer_id = (u16)((pmdev->pdev->bus->number << 8) | PCI_SLOT(pmdev->pdev->devfn));
+ mlx5_query_nic_vport_system_image_guid(fmdev, &fsystem_guid);
+ mlx5_query_nic_vport_system_image_guid(pmdev, &psystem_guid);
- return (func_id == peer_id);
+ return (fsystem_guid == psystem_guid);
}
static int parse_tc_nic_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
--
2.17.0
^ permalink raw reply related
* [PATCH net 0/2] Mellanox, mlx5e fixes 2018-08-07
From: Saeed Mahameed @ 2018-08-08 22:48 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Saeed Mahameed
Hi Dave,
I know it is late into 4.18 release, and this is why I am submitting
only two mlx5e ethernet fixes.
The first one from Or, is needed for -stable and it fixes hairpin
for "same device" check.
The second fix is a non risk fix from Huy which cleans up and improves
error return value reporting for dcbnl_ieee_setapp.
For -stable v4.16
- net/mlx5e: Properly check if hairpin is possible between two functions
Thanks,
Saeed.
---
Huy Nguyen (1):
net/mlx5e: Cleanup of dcbnl related fields
Or Gerlitz (1):
net/mlx5e: Properly check if hairpin is possible between two functions
drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 --
.../ethernet/mellanox/mlx5/core/en_dcbnl.c | 30 +++++++------------
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 8 ++---
3 files changed, 15 insertions(+), 25 deletions(-)
--
2.17.0
^ permalink raw reply
* Question about inode_permission() check in unix_find_other()
From: TongZhang @ 2018-08-09 1:09 UTC (permalink / raw)
To: davem, garsilva, matthew, willemb, netdev, linux-kernel
Cc: wenbo.s, ahmedmoneeb
Hi Networking Subsystem Maintainers,
I have a quick question about inode_permission() check in unix_find_other(),
I was doing a code review on net/unix/af_unix.c and found out that the inode_permission() check is confusing here.
Since the unix socket is presented as a file, and will be permission checked by VFS when user open it,
why do we still need the inode_permission() check in unix_find_other() anyway?
Thank you,
- Tong
^ permalink raw reply
* Re: [RFC net-next 00/15] net: A socket API for LoRa
From: Andreas Färber @ 2018-08-08 22:42 UTC (permalink / raw)
To: Alan Cox
Cc: Jian-Hong Pan, netdev, linux-arm-kernel, linux-kernel, Jiri Pirko,
Marcel Holtmann, David S. Miller, Matthias Brugger, Janus Piwek,
Michael Röder, Dollar Chen, Ken Yu, Konstantin Böhm,
Jan Jongboom, Jon Ortego, contact, Ben Whitten, Brian Ray, lora,
Alexander Graf
In-Reply-To: <20180808213640.10a1d76f@alans-desktop>
Am 08.08.2018 um 22:36 schrieb Alan Cox:
> On Sun, 5 Aug 2018 02:11:25 +0200
> Andreas Färber <afaerber@suse.de> wrote:
>> Am 03.07.2018 um 17:11 schrieb Jian-Hong Pan:
>>> 2018-07-01 19:07 GMT+08:00 Andreas Färber <afaerber@suse.de>:
>> LoRa radio channels being half-duplex, we'd need to stop receiving in
>> ndo_start_xmit and re-start receiving in the TX interrupt handler AFAIU.
>> Yes, it's ugly - one reason I haven't implemented RX in sx1276 yet.
>
> Why - the signal is still floating around in the air, you can't unhear it
> at the antenna.
Why what? :)
> If a given piece of hardware needs to flip between RX
> and TX mode then it should be handled by that driver.
Yes, and we are talking about that concrete sx1276 driver here, whose
chipset has a state machine that only allows either rx or tx and also
has standby and sleep modes with differing levels of data retention.
The sx1301 is a different beast (and driver) and may allow both.
In any case, the ndo_start_xmit hook is in each device driver. But the
recvmsg hook I mentioned is at protocol layer, which exactly is the
layering problem I see.
> (Some ancient ethernet cards do this btw.. they can't listen and transmit
> at the same time)
So when do they start receiving?
The issue here was that my original description, which you appear to
have cut, suggested a continuous listen mode, interrupted by transmit.
Jian-Hong didn't like that, with reference to the LoRaWAN spec that
supposedly asks for only being in receive mode when expecting a message,
likely to save on battery. So the question is, could we cleanly
implement receiving only when the user asks us to, or is that a no-go?
>>> - We can have a pre-defined table according to LoRaWAN Regional Parameters.
>>> - Device driver declares the hardware's capability, for example
>>> frequency, TX power. And then registers as a LoRaWAN compatible
>>> device.
>>
>> That sounds like a layering violation. We rather need to expose all
>> these tunable parameters individually at the LoRa layer, allowing the
>> LoRaWAN layer to configure them as it pleases. Not the other direction.
>> That still leaves my above question 4) open of how to implement each.
>
> Wifi already has the general policy database for the various existing
> protocols. Please take a look at the CRDA agent and how it hooks into
> wireless.It might need to some tweaking but it would be odd to have
> different configuration schemes for different wifi protocols.
CRDA/wireless-regdb had been brought up and I've been pointed to nl80211
and nl802154, which I've tried to make sense of for my initial nllora.
Admittedly with limited success, as that code is slightly complex.
We also seemed to have consensus here that we _should_ be reusing
wireless-regdb but would need to extend it 1.) with sub-GHz frequency
bands and 2.) duty-cycle limits for some of those bands. No maintainer
commented on that so far. Thus I am working in tiny steps on providing
netlink-layer commands in nllora that can dispatch the individual radio
settings to drivers, which then upper layers can instrument as needed.
And making my very first steps with netlink here, it appeared as if each
technology has its own enums of commands and attributes, so I don't see
how to reuse anything from Wifi here apart from some design inspiration.
>> The use case I have in mind is this: User A opens a LoRaWAN socket and
>> using maclorawan sends a packet P1. Here the LoRaWAN Regional Parameters
>> and LoRaWAN Sync Word need to be applied.
>> User B then opens a pure LoRa socket and transmits a packet P1' with
>> different Sync Word, SF, BW, CR, etc.
>> Afterwards user A wants to send another packet P2 via LoRaWAN - this
>> needs to use the same LoRaWAN settings as before, not those used for
>> LoRa in between. Therefore I was thinking about socket-level options, as
>> netlink operations would be device-wide, with undesired side-effects.
>
> Agreed
>
>> Obviously in that scenario not both users can receive at the same time.
>
> That's a hardware question. Imagine a software defined radio. If your
> limitation wouldn't exist in a pure software defined radio then it's
> almost certainly a device level detal.
An SDR would not be using this sx1276 device driver, I imagine.
In fact I would expect an SDR device not to be in drivers/net/lora/ at
all but to live in drivers/net/sdr/ and to consume ETH_P_LORA etc. skbs
and just do the right thing for them depending on their type...
>> interface but cleanly distinguished as ETH_P_GFSK or something.
>> For example, the Chistera Pi HAT has both an RFM95W and an RFM22 module.
>
> Agreed if you can flip per packet.
The SX127x can - it might involve delays of course.
The SX130x doesn't even need to flip for sending AFAICT, it's just
metadata after the payload in the FIFO.
>> The next question arising is how the user would create such an skb. Just
>> like I was hesitant about PF_LORAWAN originally, I'd like to avoid
>> polluting the PF_ number space for each of these. Maybe have one PF_FSK
>> as equivalent to PF_LORA and then have either a socket option or
>> sockaddr field to select the modulation variants? Not sure how exactly
>> those others differ from each other, that's why I tried to postpone the
>> FSK topic and to focus on LoRa first - b) below.
>>
>> At this point we could also argue whether we need a PF_LORA at all or
>> rather just some generic PF_RADIO with lots of options stored in its
>> sockaddr.
>
> What matters most is mux/demux.
>
> If you've got something listening to data but without the structure
> needed to identify multiple listeners and split out the data meaningfully
> to those listeners according to parts of the packet then you've got no
> reason to make it a protocol just use SOCK_PACKET and if need be BPF.
Sorry, that doesn't parse for me. SOCK_PACKET must be a protocol on some
PF_ protocol family, no? Are you suggesting I use SOCK_PACKET instead of
SOCK_DGRAM in what is now net/lora/dgram.c? Or are you saying there's
some generic implementation that we can reuse and scratch mine?
> The reason we have a socket layer not /dev/ethernet0 is that it's
> meaningful to divide messages up into flows, and to partition those flows
> securely amongst multiple consumers/generators.
For me the distinction is that a /dev/whatever0 would seem more suited
for a stream of data to read/write, whereas sockets give us a bounded
skb for packets at device driver level.
These PHYs all broadcast something over the antenna when sending, with
any addressing of listeners or senders being optional and MAC-specific,
apart from the LoRa/FSK SyncWord as well as the various frequency etc.
settings that determine what the receiver listens for.
None of these PHYs define any mechanism like EtherType through which to
identify upper-layer protocols.
So in a way, listening is always in a promiscuous mode, and I guess we
would need to try to parse each incoming packet as e.g. a LoRaWAN packet
and just give up if it's too short or checksums don't match. Only at the
layer of LoRaWAN and competing proprietary or custom protocols can we
split received packets out to individual listeners.
Does that give us any further clues for the design discussion here?
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH net-next] rds: avoid lock hierarchy violation between m_rs_lock and rs_recv_lock
From: Sowmini Varadhan @ 2018-08-08 22:18 UTC (permalink / raw)
To: Santosh Shilimkar; +Cc: netdev, davem, rds-devel
In-Reply-To: <392e9286-e98c-1dbe-d598-9afca1818cf6@oracle.com>
On (08/08/18 14:51), Santosh Shilimkar wrote:
> This bug doesn't make sense since two different transports are using
> same socket (Loop and rds_tcp) and running together.
> For same transport, such race can't happen with MSG_ON_SOCK flag.
> CPU1-> rds_loop_inc_free
> CPU0 -> rds_tcp_cork ...
>
The test is just reporting a lock hierarchy violation
As far as I can tell, this wasn't an actual deadlock that happened
because as you point out, either a socket has the rds_tcp transport
or the rds_loop transport, so this particular pair of stack traces
would not happen with the code as it exists today.
but there is a valid lock hierachy violation here, and
imho it's a good idea to get that cleaned up.
It also avoids needlessly holding down the rs_recv_lock
when doing an rds_inc_put.
--Sowmini
^ permalink raw reply
* Re: [PATCH] net: phy: sftp: print debug message with text, not numbers
From: Russell King - ARM Linux @ 2018-08-08 22:16 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, netdev, Florian Fainelli
In-Reply-To: <1533754452-1633-1-git-send-email-andrew@lunn.ch>
You might want to fix the subject line.
On Wed, Aug 08, 2018 at 08:54:12PM +0200, Andrew Lunn wrote:
> Convert the state numbers, device state, etc from numbers to strings
> when printing debug messages.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> drivers/net/phy/sfp.c | 76 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 5661226cf75b..4637d980310e 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -60,6 +60,69 @@ enum {
> SFP_S_TX_DISABLE,
> };
>
> +static const char * const mod_state_strings[] = {
> + [SFP_MOD_EMPTY] = "empty",
> + [SFP_MOD_PROBE] = "probe",
> + [SFP_MOD_HPOWER] = "hpower",
> + [SFP_MOD_PRESENT] = "present",
> + [SFP_MOD_ERROR] = "error",
> +};
> +
> +static const char *mod_state_to_str(unsigned short mod_state)
> +{
> + if (mod_state >= ARRAY_SIZE(mod_state_strings))
> + return "Unknown module state";
> + return mod_state_strings[mod_state];
> +}
> +
> +static const char * const dev_state_strings[] = {
> + [SFP_DEV_DOWN] = "down",
> + [SFP_DEV_UP] = "up",
> +};
> +
> +static const char *dev_state_to_str(unsigned short dev_state)
> +{
> + if (dev_state >= ARRAY_SIZE(dev_state_strings))
> + return "Unknown device state";
> + return dev_state_strings[dev_state];
> +}
> +
> +static const char * const event_strings[] = {
> + [SFP_E_INSERT] = "insert",
> + [SFP_E_REMOVE] = "remove",
> + [SFP_E_DEV_DOWN] = "dev_down",
> + [SFP_E_DEV_UP] = "dev_up",
> + [SFP_E_TX_FAULT] = "tx_fault",
> + [SFP_E_TX_CLEAR] = "tx_clear",
> + [SFP_E_LOS_HIGH] = "los_high",
> + [SFP_E_LOS_LOW] = "los_low",
> + [SFP_E_TIMEOUT] = "timeout",
> +};
> +
> +static const char *event_to_str(unsigned short event)
> +{
> + if (event >= ARRAY_SIZE(event_strings))
> + return "Unknown event";
> + return event_strings[event];
> +}
> +
> +static const char * const sm_state_strings[] = {
> + [SFP_S_DOWN] = "down",
> + [SFP_S_INIT] = "init",
> + [SFP_S_WAIT_LOS] = "wait_los",
> + [SFP_S_LINK_UP] = "link_up",
> + [SFP_S_TX_FAULT] = "tx_fault",
> + [SFP_S_REINIT] = "reinit",
> + [SFP_S_TX_DISABLE] = "rx_disable",
> +};
> +
> +static const char *sm_state_to_str(unsigned short sm_state)
> +{
> + if (sm_state >= ARRAY_SIZE(sm_state_strings))
> + return "Unknown state";
> + return sm_state_strings[sm_state];
> +}
> +
> static const char *gpio_of_names[] = {
> "mod-def0",
> "los",
> @@ -1388,8 +1451,11 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event)
> {
> mutex_lock(&sfp->sm_mutex);
>
> - dev_dbg(sfp->dev, "SM: enter %u:%u:%u event %u\n",
> - sfp->sm_mod_state, sfp->sm_dev_state, sfp->sm_state, event);
> + dev_dbg(sfp->dev, "SM: enter %s:%s:%s event %s\n",
> + mod_state_to_str(sfp->sm_mod_state),
> + dev_state_to_str(sfp->sm_dev_state),
> + sm_state_to_str(sfp->sm_state),
> + event_to_str(event));
>
> /* This state machine tracks the insert/remove state of
> * the module, and handles probing the on-board EEPROM.
> @@ -1520,8 +1586,10 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event)
> break;
> }
>
> - dev_dbg(sfp->dev, "SM: exit %u:%u:%u\n",
> - sfp->sm_mod_state, sfp->sm_dev_state, sfp->sm_state);
> + dev_dbg(sfp->dev, "SM: exit %s:%s:%s\n",
> + mod_state_to_str(sfp->sm_mod_state),
> + dev_state_to_str(sfp->sm_dev_state),
> + sm_state_to_str(sfp->sm_state));
>
> mutex_unlock(&sfp->sm_mutex);
> }
> --
> 2.18.0
>
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply
* Re: [PATCH net-next] rds: avoid lock hierarchy violation between m_rs_lock and rs_recv_lock
From: Santosh Shilimkar @ 2018-08-08 21:51 UTC (permalink / raw)
To: Sowmini Varadhan, netdev; +Cc: davem, rds-devel
In-Reply-To: <1533761833-106379-1-git-send-email-sowmini.varadhan@oracle.com>
On 8/8/2018 1:57 PM, Sowmini Varadhan wrote:
> The following deadlock, reported by syzbot, can occur if CPU0 is in
> rds_send_remove_from_sock() while CPU1 is in rds_clear_recv_queue()
>
> CPU0 CPU1
> ---- ----
> lock(&(&rm->m_rs_lock)->rlock);
> lock(&rs->rs_recv_lock);
> lock(&(&rm->m_rs_lock)->rlock);
> lock(&rs->rs_recv_lock);
>
> The deadlock should be avoided by moving the messages from the
> rs_recv_queue into a tmp_list in rds_clear_recv_queue() under
> the rs_recv_lock, and then dropping the refcnt on the messages
> in the tmp_list (potentially resulting in rds_message_purge())
> after dropping the rs_recv_lock.
>
> The same lock hierarchy violation also exists in rds_still_queued()
> and should be avoided in a similar manner
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> Reported-by: syzbot+52140d69ac6dc6b927a9@syzkaller.appspotmail.com
> ---
This bug doesn't make sense since two different transports are using
same socket (Loop and rds_tcp) and running together.
For same transport, such race can't happen with MSG_ON_SOCK flag.
CPU1-> rds_loop_inc_free
CPU0 -> rds_tcp_cork ...
I need to understand this test better.
Regards,
Santosh
^ permalink raw reply
* [Patch net-next] net_sched: fix a potential out-of-bound access
From: Cong Wang @ 2018-08-08 21:32 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Jiri Pirko, Vlad Buslov
In tca_action_gd(), when tcf_action_get_1() fails in the middle
of the loop, tcf_action_put_many(&actions[acts_deleted]) is
called to cleanup.
But inside tcf_action_put_many() it still iterates from
0 to TCA_ACT_MAX_PRIO, so inside it would be:
&actions[acts_deleted][0]...&actions[acts_deleted][MAX_PRIO]
Then the overall of the result is:
actions[acts_deleted]...actions[acts_deleted + MAX_PRIO]
We have a potential out-of-bound access when acts_deleted > 1.
acts_deleted is completely unnecessary since tcf_action_put_many()
checks against NULL pointer.
Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers to actions")
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/act_api.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 229d63c99be2..36549bc7ce78 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1176,7 +1176,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
}
static int tcf_action_delete(struct net *net, struct tc_action *actions[],
- int *acts_deleted, struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack)
{
u32 act_index;
int ret, i;
@@ -1196,20 +1196,16 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[],
} else {
/* now do the delete */
ret = ops->delete(net, act_index);
- if (ret < 0) {
- *acts_deleted = i + 1;
+ if (ret < 0)
return ret;
- }
}
}
- *acts_deleted = i;
return 0;
}
static int
tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
- int *acts_deleted, u32 portid, size_t attr_size,
- struct netlink_ext_ack *extack)
+ u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
{
int ret;
struct sk_buff *skb;
@@ -1227,7 +1223,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
}
/* now do the delete */
- ret = tcf_action_delete(net, actions, acts_deleted, extack);
+ ret = tcf_action_delete(net, actions, extack);
if (ret < 0) {
NL_SET_ERR_MSG(extack, "Failed to delete TC action");
kfree_skb(skb);
@@ -1250,7 +1246,6 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
struct tc_action *act;
size_t attr_size = 0;
struct tc_action *actions[TCA_ACT_MAX_PRIO + 1] = {};
- int acts_deleted = 0;
ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
if (ret < 0)
@@ -1280,14 +1275,14 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
if (event == RTM_GETACTION)
ret = tcf_get_notify(net, portid, n, actions, event, extack);
else { /* delete */
- ret = tcf_del_notify(net, n, actions, &acts_deleted, portid,
+ ret = tcf_del_notify(net, n, actions, portid,
attr_size, extack);
if (ret)
goto err;
return ret;
}
err:
- tcf_action_put_many(&actions[acts_deleted]);
+ tcf_action_put_many(actions);
return ret;
}
--
2.14.4
^ permalink raw reply related
* [net PATCH 3/3] xdp: fix bug in devmap teardown code path
From: Jesper Dangaard Brouer @ 2018-08-08 21:00 UTC (permalink / raw)
To: netdev, Jesper Dangaard Brouer
Cc: Daniel Borkmann, Alexei Starovoitov, jhsiao
In-Reply-To: <153376197849.14272.8201612461878004477.stgit@firesoul>
Like cpumap teardown, the devmap teardown code also flush remaining
xdp_frames, via bq_xmit_all() in case map entry is removed. The code
can call xdp_return_frame_rx_napi, from the the wrong context, in-case
ndo_xdp_xmit() fails.
Fixes: 389ab7f01af9 ("xdp: introduce xdp_return_frame_rx_napi")
Fixes: 735fc4054b3a ("xdp: change ndo_xdp_xmit API to support bulking")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
kernel/bpf/devmap.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index d361fc1e3bf3..750d45edae79 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -217,7 +217,8 @@ void __dev_map_insert_ctx(struct bpf_map *map, u32 bit)
}
static int bq_xmit_all(struct bpf_dtab_netdev *obj,
- struct xdp_bulk_queue *bq, u32 flags)
+ struct xdp_bulk_queue *bq, u32 flags,
+ bool in_napi_ctx)
{
struct net_device *dev = obj->dev;
int sent = 0, drops = 0, err = 0;
@@ -254,7 +255,10 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
struct xdp_frame *xdpf = bq->q[i];
/* RX path under NAPI protection, can return frames faster */
- xdp_return_frame_rx_napi(xdpf);
+ if (likely(in_napi_ctx))
+ xdp_return_frame_rx_napi(xdpf);
+ else
+ xdp_return_frame(xdpf);
drops++;
}
goto out;
@@ -286,7 +290,7 @@ void __dev_map_flush(struct bpf_map *map)
__clear_bit(bit, bitmap);
bq = this_cpu_ptr(dev->bulkq);
- bq_xmit_all(dev, bq, XDP_XMIT_FLUSH);
+ bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, true);
}
}
@@ -316,7 +320,7 @@ static int bq_enqueue(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf,
struct xdp_bulk_queue *bq = this_cpu_ptr(obj->bulkq);
if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
- bq_xmit_all(obj, bq, 0);
+ bq_xmit_all(obj, bq, 0, true);
/* Ingress dev_rx will be the same for all xdp_frame's in
* bulk_queue, because bq stored per-CPU and must be flushed
@@ -385,7 +389,7 @@ static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
__clear_bit(dev->bit, bitmap);
bq = per_cpu_ptr(dev->bulkq, cpu);
- bq_xmit_all(dev, bq, XDP_XMIT_FLUSH);
+ bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, false);
}
}
}
^ permalink raw reply related
* [net PATCH 2/3] samples/bpf: xdp_redirect_cpu adjustment to reproduce teardown race easier
From: Jesper Dangaard Brouer @ 2018-08-08 21:00 UTC (permalink / raw)
To: netdev, Jesper Dangaard Brouer
Cc: Daniel Borkmann, Alexei Starovoitov, jhsiao
In-Reply-To: <153376197849.14272.8201612461878004477.stgit@firesoul>
The teardown race in cpumap is really hard to reproduce. These changes
makes it easier to reproduce, for QA.
The --stress-mode now have a case of a very small queue size of 8, that helps
to trigger teardown flush to encounter a full queue, which results in calling
xdp_return_frame API, in a non-NAPI protect context.
Also increase MAX_CPUS, as my QA department have larger machines than me.
Tested-by: Jean-Tsung Hsiao <jhsiao@redhat.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
samples/bpf/xdp_redirect_cpu_kern.c | 2 +-
samples/bpf/xdp_redirect_cpu_user.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/samples/bpf/xdp_redirect_cpu_kern.c b/samples/bpf/xdp_redirect_cpu_kern.c
index 8cb703671b04..0cc3d71057f0 100644
--- a/samples/bpf/xdp_redirect_cpu_kern.c
+++ b/samples/bpf/xdp_redirect_cpu_kern.c
@@ -14,7 +14,7 @@
#include <uapi/linux/bpf.h>
#include "bpf_helpers.h"
-#define MAX_CPUS 12 /* WARNING - sync with _user.c */
+#define MAX_CPUS 64 /* WARNING - sync with _user.c */
/* Special map type that can XDP_REDIRECT frames to another CPU */
struct bpf_map_def SEC("maps") cpu_map = {
diff --git a/samples/bpf/xdp_redirect_cpu_user.c b/samples/bpf/xdp_redirect_cpu_user.c
index f6efaefd485b..4b4d78fffe30 100644
--- a/samples/bpf/xdp_redirect_cpu_user.c
+++ b/samples/bpf/xdp_redirect_cpu_user.c
@@ -19,7 +19,7 @@ static const char *__doc__ =
#include <arpa/inet.h>
#include <linux/if_link.h>
-#define MAX_CPUS 12 /* WARNING - sync with _kern.c */
+#define MAX_CPUS 64 /* WARNING - sync with _kern.c */
/* How many xdp_progs are defined in _kern.c */
#define MAX_PROG 5
@@ -527,7 +527,7 @@ static void stress_cpumap(void)
* procedure.
*/
create_cpu_entry(1, 1024, 0, false);
- create_cpu_entry(1, 128, 0, false);
+ create_cpu_entry(1, 8, 0, false);
create_cpu_entry(1, 16000, 0, false);
}
^ permalink raw reply related
* [net PATCH 1/3] xdp: fix bug in cpumap teardown code path
From: Jesper Dangaard Brouer @ 2018-08-08 21:00 UTC (permalink / raw)
To: netdev, Jesper Dangaard Brouer
Cc: Daniel Borkmann, Alexei Starovoitov, jhsiao
In-Reply-To: <153376197849.14272.8201612461878004477.stgit@firesoul>
When removing a cpumap entry, a number of syncronization steps happen.
Eventually the teardown code __cpu_map_entry_free is invoked from/via
call_rcu.
The teardown code __cpu_map_entry_free() flushes remaining xdp_frames,
by invoking bq_flush_to_queue, which calls xdp_return_frame_rx_napi().
The issues is that the teardown code is not running in the RX NAPI
code path. Thus, it is not allowed to invoke the NAPI variant of
xdp_return_frame.
This bug was found and triggered by using the --stress-mode option to
the samples/bpf program xdp_redirect_cpu. It is hard to trigger,
because the ptr_ring have to be full and cpumap bulk queue max
contains 8 packets, and a remote CPU is racing to empty the ptr_ring
queue.
Fixes: 389ab7f01af9 ("xdp: introduce xdp_return_frame_rx_napi")
Tested-by: Jean-Tsung Hsiao <jhsiao@redhat.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
kernel/bpf/cpumap.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index e0918d180f08..46f5f29605d4 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -69,7 +69,7 @@ struct bpf_cpu_map {
};
static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
- struct xdp_bulk_queue *bq);
+ struct xdp_bulk_queue *bq, bool in_napi_ctx);
static u64 cpu_map_bitmap_size(const union bpf_attr *attr)
{
@@ -375,7 +375,7 @@ static void __cpu_map_entry_free(struct rcu_head *rcu)
struct xdp_bulk_queue *bq = per_cpu_ptr(rcpu->bulkq, cpu);
/* No concurrent bq_enqueue can run at this point */
- bq_flush_to_queue(rcpu, bq);
+ bq_flush_to_queue(rcpu, bq, false);
}
free_percpu(rcpu->bulkq);
/* Cannot kthread_stop() here, last put free rcpu resources */
@@ -558,7 +558,7 @@ const struct bpf_map_ops cpu_map_ops = {
};
static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
- struct xdp_bulk_queue *bq)
+ struct xdp_bulk_queue *bq, bool in_napi_ctx)
{
unsigned int processed = 0, drops = 0;
const int to_cpu = rcpu->cpu;
@@ -578,7 +578,10 @@ static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
err = __ptr_ring_produce(q, xdpf);
if (err) {
drops++;
- xdp_return_frame_rx_napi(xdpf);
+ if (likely(in_napi_ctx))
+ xdp_return_frame_rx_napi(xdpf);
+ else
+ xdp_return_frame(xdpf);
}
processed++;
}
@@ -598,7 +601,7 @@ static int bq_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_frame *xdpf)
struct xdp_bulk_queue *bq = this_cpu_ptr(rcpu->bulkq);
if (unlikely(bq->count == CPU_MAP_BULK_SIZE))
- bq_flush_to_queue(rcpu, bq);
+ bq_flush_to_queue(rcpu, bq, true);
/* Notice, xdp_buff/page MUST be queued here, long enough for
* driver to code invoking us to finished, due to driver
@@ -661,7 +664,7 @@ void __cpu_map_flush(struct bpf_map *map)
/* Flush all frames in bulkq to real queue */
bq = this_cpu_ptr(rcpu->bulkq);
- bq_flush_to_queue(rcpu, bq);
+ bq_flush_to_queue(rcpu, bq, true);
/* If already running, costs spin_lock_irqsave + smb_mb */
wake_up_process(rcpu->kthread);
^ permalink raw reply related
* [net PATCH 0/3] Fix two teardown bugs for BPF maps cpumap and devmap
From: Jesper Dangaard Brouer @ 2018-08-08 21:00 UTC (permalink / raw)
To: netdev, Jesper Dangaard Brouer
Cc: Daniel Borkmann, Alexei Starovoitov, jhsiao
Removing entries from cpumap and devmap, goes through a number of
syncronization steps to make sure no new xdp_frames can be enqueued.
But there is a small chance, that xdp_frames remains which have not
been flushed/processed yet. Flushing these during teardown, happens
from RCU context and not as usual under RX NAPI context.
The optimization introduced in commt 389ab7f01af9 ("xdp: introduce
xdp_return_frame_rx_napi"), missed that the flush operation can also
be called from RCU context. Thus, we cannot always use the
xdp_return_frame_rx_napi call, which take advantage of the protection
provided by XDP RX running under NAPI protection.
The samples/bpf xdp_redirect_cpu have a --stress-mode, that is
adjusted to easier reproduce (verified by Red Hat QA).
---
Jesper Dangaard Brouer (3):
xdp: fix bug in cpumap teardown code path
samples/bpf: xdp_redirect_cpu adjustment to reproduce teardown race easier
xdp: fix bug in devmap teardown code path
kernel/bpf/cpumap.c | 15 +++++++++------
kernel/bpf/devmap.c | 14 +++++++++-----
samples/bpf/xdp_redirect_cpu_kern.c | 2 +-
samples/bpf/xdp_redirect_cpu_user.c | 4 ++--
4 files changed, 21 insertions(+), 14 deletions(-)
^ permalink raw reply
* [PATCH net-next] rds: avoid lock hierarchy violation between m_rs_lock and rs_recv_lock
From: Sowmini Varadhan @ 2018-08-08 20:57 UTC (permalink / raw)
To: netdev, sowmini.varadhan
Cc: davem, rds-devel, sowmini.varadhan, santosh.shilimkar
The following deadlock, reported by syzbot, can occur if CPU0 is in
rds_send_remove_from_sock() while CPU1 is in rds_clear_recv_queue()
CPU0 CPU1
---- ----
lock(&(&rm->m_rs_lock)->rlock);
lock(&rs->rs_recv_lock);
lock(&(&rm->m_rs_lock)->rlock);
lock(&rs->rs_recv_lock);
The deadlock should be avoided by moving the messages from the
rs_recv_queue into a tmp_list in rds_clear_recv_queue() under
the rs_recv_lock, and then dropping the refcnt on the messages
in the tmp_list (potentially resulting in rds_message_purge())
after dropping the rs_recv_lock.
The same lock hierarchy violation also exists in rds_still_queued()
and should be avoided in a similar manner
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Reported-by: syzbot+52140d69ac6dc6b927a9@syzkaller.appspotmail.com
---
net/rds/recv.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/rds/recv.c b/net/rds/recv.c
index 504cd6b..1cf7072 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -429,6 +429,7 @@ static int rds_still_queued(struct rds_sock *rs, struct rds_incoming *inc,
struct sock *sk = rds_rs_to_sk(rs);
int ret = 0;
unsigned long flags;
+ bool drop_ref = false;
write_lock_irqsave(&rs->rs_recv_lock, flags);
if (!list_empty(&inc->i_item)) {
@@ -439,11 +440,13 @@ static int rds_still_queued(struct rds_sock *rs, struct rds_incoming *inc,
-be32_to_cpu(inc->i_hdr.h_len),
inc->i_hdr.h_dport);
list_del_init(&inc->i_item);
- rds_inc_put(inc);
+ drop_ref = true;
}
}
write_unlock_irqrestore(&rs->rs_recv_lock, flags);
+ if (drop_ref)
+ rds_inc_put(inc);
rdsdebug("inc %p rs %p still %d dropped %d\n", inc, rs, ret, drop);
return ret;
}
@@ -751,16 +754,20 @@ void rds_clear_recv_queue(struct rds_sock *rs)
struct sock *sk = rds_rs_to_sk(rs);
struct rds_incoming *inc, *tmp;
unsigned long flags;
+ LIST_HEAD(tmp_list);
write_lock_irqsave(&rs->rs_recv_lock, flags);
list_for_each_entry_safe(inc, tmp, &rs->rs_recv_queue, i_item) {
rds_recv_rcvbuf_delta(rs, sk, inc->i_conn->c_lcong,
-be32_to_cpu(inc->i_hdr.h_len),
inc->i_hdr.h_dport);
+ list_move_tail(&inc->i_item, &tmp_list);
+ }
+ write_unlock_irqrestore(&rs->rs_recv_lock, flags);
+ list_for_each_entry_safe(inc, tmp, &tmp_list, i_item) {
list_del_init(&inc->i_item);
rds_inc_put(inc);
}
- write_unlock_irqrestore(&rs->rs_recv_lock, flags);
}
/*
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] 9p: fix NULL pointer dereferences
From: Dominique Martinet @ 2018-08-08 22:41 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Tomas Bortoli, David Miller, v9fs-developer, netdev, LKML,
syzkaller
In-Reply-To: <CACT4Y+Z+Q__-cMaQ-85H38Rs5AeFGitXoE8-Bv4pNsvat+G+xQ@mail.gmail.com>
Dmitry Vyukov wrote on Wed, Aug 08, 2018:
> > If you want to factor it in, v9fs_mount does not know which transport is
> > used, but p9_client_create does know - although the functions/structs
> > are all static so you need to check clnt->trans_mod->name with strcmp
> > and I'm honestly not sure that's better than checking in each function..
> >
> > But, as usual I'm happy as long as it works, so pick your poison :)
>
> So let's proceed with checking in each transport function?
Yes, he's done it a while ago, just didn't se the in-reply-to field to
keep part of the thread but the Reported-by was here:
http://lkml.kernel.org/r/20180727110558.5479-1-tomasbortoli@gmail.com
This is in linux-next right now as 631263a1b23c71
--
Dominique
^ permalink raw reply
* Re: [PATCH net-next] ieee802154: hwsim: fix missing unlock on error in hwsim_add_one()
From: Stefan Schmidt @ 2018-08-08 20:06 UTC (permalink / raw)
To: Wei Yongjun, Alexander Aring; +Cc: linux-wpan, netdev, kernel-janitors
In-Reply-To: <1533697839-85602-1-git-send-email-weiyongjun1@huawei.com>
Hello Wei.
On 08/08/2018 05:10 AM, Wei Yongjun wrote:
> Add the missing unlock before return from function hwsim_add_one()
> in the error handling case.
>
> Fixes: f25da51fdc38 ("ieee802154: hwsim: add replacement for fakelb")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/net/ieee802154/mac802154_hwsim.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> index f4e9205..44d398c 100644
> --- a/drivers/net/ieee802154/mac802154_hwsim.c
> +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> @@ -810,8 +810,10 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
> mutex_lock(&hwsim_phys_lock);
> if (init) {
> err = hwsim_subscribe_all_others(phy);
> - if (err < 0)
> + if (err < 0) {
> + mutex_unlock(&hwsim_phys_lock);
> goto err_reg;
> + }
> }
> list_add_tail(&phy->list, &hwsim_phys);
> mutex_unlock(&hwsim_phys_lock);
>
Signed-off-by: Stefan Schmidt <stefan@datenfreihafen.org>
regards
Stefan Schmidt
^ permalink raw reply
* Re: [PATCH net-next] ieee802154: hwsim: fix copy-paste error in hwsim_set_edge_lqi()
From: Stefan Schmidt @ 2018-08-08 20:06 UTC (permalink / raw)
To: Wei Yongjun, Alexander Aring; +Cc: linux-wpan, netdev, kernel-janitors
In-Reply-To: <1533696226-26445-1-git-send-email-weiyongjun1@huawei.com>
Hello Wei.
On 08/08/2018 04:43 AM, Wei Yongjun wrote:
> The return value from kzalloc() is not checked correctly. The
> test is done against a wrong variable. This patch fix it.
>
> Fixes: f25da51fdc38 ("ieee802154: hwsim: add replacement for fakelb")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/net/ieee802154/mac802154_hwsim.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> index 1982308..04f4100 100644
> --- a/drivers/net/ieee802154/mac802154_hwsim.c
> +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> @@ -564,7 +564,7 @@ static int hwsim_set_edge_lqi(struct sk_buff *msg, struct genl_info *info)
> }
>
> einfo = kzalloc(sizeof(*einfo), GFP_KERNEL);
> - if (!info) {
> + if (!einfo) {
> mutex_unlock(&hwsim_phys_lock);
> return -ENOMEM;
> }
>
Thanks a lot for finding and fixing these two bugs!
Signed-off-by: Stefan Schmidt <stefan@datenfreihafen.org>
regards
Stefan Schmidt
^ permalink raw reply
* Re: [PATCH net-next] ieee802154: hwsim: fix rcu handling
From: Stefan Schmidt @ 2018-08-08 20:05 UTC (permalink / raw)
To: Alexander Aring, netdev; +Cc: linux-wpan, kernel
In-Reply-To: <20180807233249.28572-1-aring@mojatatu.com>
Hello David.
I want to apologize how the submission of the ieee802154_hwsim driver
worked out. The sparse warning finally fixed by this commit came due to
an outdated sparse used on our side while kbuild was up to date. I fixed
this now and sparse has no warnings for the ieee802154_hwsim driver
anymore with this patch applied.
The other two fixes from Wei Yongjun are problems I should have spotted
during code review, but did not. No excuse, I just overlooked them.
I would, once again, ask you to apply these three directly to net-next.
I you would prefer for me to pick them up and send you a pull request
this time just let me know and I will do so tomorrow.
On 08/08/2018 01:32 AM, Alexander Aring wrote:
> This patch adds missing rcu_assign_pointer()/rcu_dereference() to used rcu
> pointers. There was already a previous commit c5d99d2b35da ("ieee802154:
> hwsim: fix rcu address annotation"), but there was more which was
> pointed out on my side by using newest sparse version.
>
> Cc: Stefan Schmidt <stefan@datenfreihafen.org>
> Fixes: f25da51fdc38 ("ieee802154: hwsim: add replacement for fakelb")
> Signed-off-by: Alexander Aring <aring@mojatatu.com>
> ---
> After I installed finally the newest sparse version I found more what kbuild
> was pointed out.
>
> I hope I did it right, not sure if I really need rcu functionality in these
> case because protection by mutex but I make sparse silent.
> At some places the resource isn't a shared resource at this moment.
>
> Anyway in my case I want to use this driver to create easily multi-hop
> scenarios, if somebody report things (or I hit it) which smells like some
> memory in this area - I will try and help to fix it.
>
> Newest sparse version from git is happy now and I hope kbuild bot as well.
>
> I tested this patch with RPOVE_RCU enabled and tried to update these settings
> while heavy loading in rcu protected xmit hotpath is occured, without any
> issues so far.
>
> Sorry again.
>
> drivers/net/ieee802154/mac802154_hwsim.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> index f4e92054f7df..53f39435321e 100644
> --- a/drivers/net/ieee802154/mac802154_hwsim.c
> +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> @@ -22,6 +22,7 @@
> #include <linux/module.h>
> #include <linux/timer.h>
> #include <linux/platform_device.h>
> +#include <linux/rtnetlink.h>
> #include <linux/netdevice.h>
> #include <linux/device.h>
> #include <linux/spinlock.h>
> @@ -110,7 +111,7 @@ static int hwsim_hw_channel(struct ieee802154_hw *hw, u8 page, u8 channel)
> pib->page = page;
> pib->channel = channel;
>
> - pib_old = phy->pib;
> + pib_old = rtnl_dereference(phy->pib);
> rcu_assign_pointer(phy->pib, pib);
> kfree_rcu(pib_old, rcu);
> return 0;
> @@ -406,7 +407,7 @@ static struct hwsim_edge *hwsim_alloc_edge(struct hwsim_phy *endpoint, u8 lqi)
> }
>
> einfo->lqi = 0xff;
> - e->info = einfo;
> + rcu_assign_pointer(e->info, einfo);
> e->endpoint = endpoint;
>
> return e;
> @@ -414,7 +415,13 @@ static struct hwsim_edge *hwsim_alloc_edge(struct hwsim_phy *endpoint, u8 lqi)
>
> static void hwsim_free_edge(struct hwsim_edge *e)
> {
> - kfree_rcu(e->info, rcu);
> + struct hwsim_edge_info *einfo;
> +
> + rcu_read_lock();
> + einfo = rcu_dereference(e->info);
> + rcu_read_unlock();
> +
> + kfree_rcu(einfo, rcu);
> kfree_rcu(e, rcu);
> }
>
> @@ -796,7 +803,7 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
> goto err_pib;
> }
>
> - phy->pib = pib;
> + rcu_assign_pointer(phy->pib, pib);
> phy->idx = idx;
> INIT_LIST_HEAD(&phy->edges);
>
> @@ -829,10 +836,17 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
>
> static void hwsim_del(struct hwsim_phy *phy)
> {
> + struct hwsim_pib *pib;
> +
> hwsim_edge_unsubscribe_me(phy);
>
> list_del(&phy->list);
> - kfree_rcu(phy->pib, rcu);
> +
> + rcu_read_lock();
> + pib = rcu_dereference(phy->pib);
> + rcu_read_unlock();
> +
> + kfree_rcu(pib, rcu);
>
> ieee802154_unregister_hw(phy->hw);
> ieee802154_free_hw(phy->hw);
>
Signed-off-by: Stefan Schmidt <stefan@datenfreihafen.org>
regards
Stefan Schmidt
^ permalink raw reply
* Re: [PATCH 24/33] net/mlx5e: Mark expected switch fall-throughs
From: Saeed Mahameed @ 2018-08-08 22:26 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: David S. Miller, Linux Netdev List, linux-kernel, Saeed Mahameed,
Leon Romanovsky, RDMA mailing list
In-Reply-To: <3c523c58a8e3f3d240d4ff9363d672fb977c9d93.1533675546.git.gustavo@embeddedor.com>
On Tue, Aug 7, 2018 at 4:25 PM, Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Addresses-Coverity-ID: 114808 ("Missing break in switch")
> Addresses-Coverity-ID: 114802 ("Missing break in switch")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Acked-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> index 1881468..ad6d471 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> @@ -91,9 +91,11 @@ bool mlx5e_xdp_handle(struct mlx5e_rq *rq, struct mlx5e_dma_info *di,
> return true;
> default:
> bpf_warn_invalid_xdp_action(act);
> + /* fall through */
> case XDP_ABORTED:
> xdp_abort:
> trace_xdp_exception(rq->netdev, prog, act);
> + /* fall through */
> case XDP_DROP:
> rq->stats->xdp_drop++;
> return true;
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH bpf-next 12/13] docs: net: Fix various minor typos
From: Tobin C. Harding @ 2018-08-08 22:21 UTC (permalink / raw)
To: Markus Heiser
Cc: Daniel Borkmann, Jonathan Corbet, Alexei Starovoitov,
David S. Miller, linux-doc, netdev, linux-kernel
In-Reply-To: <1cfb4fb3406621c1ffd98e8072f2cc38812107d8.camel@darmarit.de>
On Wed, Aug 08, 2018 at 06:26:50PM +0200, Markus Heiser wrote:
>
>
> Am Mittwoch, den 08.08.2018, 15:45 +0200 schrieb Daniel Borkmann:
> > On 08/08/2018 03:23 PM, Jonathan Corbet wrote:
> > > On Wed, 8 Aug 2018 11:42:48 +1000
> > > "Tobin C. Harding" <me@tobin.cc> wrote:
> > >
> > > > Thanks for doing such a careful review that you noticed this. I'm
> > > > working on this more ATM and I've moved the document to use double
> > > > spaces between _all_ full stops. Currently the document uses mostly
> > > > single spaces but there are some sections with double space. The
> > > > internet tells me this is a 'style' issue not a rule. And I've seen
> > > > single and double spacing in tree and do not know if one is favoured.
> > > >
> > > > Do you care?
> > >
> > > I'm not Daniel, but let me just say that, for docs in general, I'm
> > > absolutely uninterested in patches adjusting the number of spaces after
> > > periods. It really doesn't matter one way or the other, and I don't think
> > > we benefit from that kind of churn.
> >
> > Yep, agree.
>
> FWIW: even if it is not a patch worth, 'fill-paragraph' within emacs benefit
> from.
That's why I originally started using double spaces :)
thanks,
Tobin.
^ permalink raw reply
* Re: [PATCH net-next] decnet: fix using plain integer as NULL warning
From: Kees Cook @ 2018-08-08 22:03 UTC (permalink / raw)
To: YueHaibing
Cc: David S. Miller, LKML, Network Development, Eric Dumazet,
linux-decnet-user
In-Reply-To: <20180808115932.12944-1-yuehaibing@huawei.com>
On Wed, Aug 8, 2018 at 4:59 AM, YueHaibing <yuehaibing@huawei.com> wrote:
> Fixes the following sparse warning:
> net/decnet/dn_route.c:407:30: warning: Using plain integer as NULL pointer
> net/decnet/dn_route.c:1923:22: warning: Using plain integer as NULL pointer
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* [PATCH bpf 0/3] Couple of sockmap fixes
From: Daniel Borkmann @ 2018-08-08 19:33 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: netdev, ast, john.fastabend
In-Reply-To: <20180808191259.r5akjmsv5mdzmpje@ast-mbp>
On 08/08/2018 09:13 PM, Alexei Starovoitov wrote:
> On Wed, Aug 08, 2018 at 07:23:12PM +0200, Daniel Borkmann wrote:
>> Two sockmap fixes in bpf_tcp_sendmsg(), and one fix for the
>> sockmap kernel selftest. Thanks!
>
> test_sockmap jumped from 10 to 47 seconds :(
Agree, it's unfortunate, but otherwise the tests keep failing since the fix
in patch 1 does propagate errors now. So while test_sockmap before that fix
was succeeding for me, it correctly exposed the EPIPE now due to the timeout,
so it's a minimal workaround (for the time being).
> but applied anyway.
> Thanks
Thanks!
^ permalink raw reply
* Re: [PATCH] net: phy: sftp: print debug message with text, not numbers
From: Andrew Lunn @ 2018-08-08 19:18 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev, Russell King
In-Reply-To: <6f042c52-fdc9-c90c-82ab-2a7b38c04583@gmail.com>
On Wed, Aug 08, 2018 at 12:02:56PM -0700, Florian Fainelli wrote:
> On 08/08/2018 11:54 AM, Andrew Lunn wrote:
> > Convert the state numbers, device state, etc from numbers to strings
> > when printing debug messages.
>
> I had a similar patch locally that I used for initial troubleshooting,
> which I might even have shared with Russell at some point, though now I
> can't remember if he was okay or not with the idea. This is much more
> readable IMHO so:
Yes, I should of included an example:
[ 87.359862] sfp sff3: SM: enter present:down:down event dev_up
[ 87.359875] sfp sff3: tx disable 1 -> 0
[ 87.359900] sfp sff3: SM: exit present:up:init
[ 87.360099] IPv6: ADDRCONF(NETDEV_UP): sff3: link is not ready
[ 87.661662] sfp sff3: SM: enter present:up:init event timeout
[ 87.661697] sfp sff3: SM: exit present:up:link_up
[ 87.661827] mv88e6085 0.2:00 sff3: Link is Up - 1Gbps/Full - flow control off
Andrew
^ permalink raw reply
* possible deadlock in team_vlan_rx_add_vid
From: syzbot @ 2018-08-08 21:39 UTC (permalink / raw)
To: davem, jiri, linux-kernel, netdev, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 0b5b1f9a78b5 Merge tag 'for-linus' of git://git.kernel.org..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1514a09c400000
kernel config: https://syzkaller.appspot.com/x/.config?x=2dc0cd7c2eefb46f
dashboard link: https://syzkaller.appspot.com/bug?extid=bd051aba086537515cdb
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=16b10a54400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+bd051aba086537515cdb@syzkaller.appspotmail.com
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
============================================
WARNING: possible recursive locking detected
4.18.0-rc7+ #176 Not tainted
--------------------------------------------
syz-executor4/6391 is trying to acquire lock:
(____ptrval____) (&team->lock){+.+.}, at: team_vlan_rx_add_vid+0x3b/0x1e0
drivers/net/team/team.c:1868
but task is already holding lock:
(____ptrval____) (&team->lock){+.+.}, at: team_add_slave+0xdb/0x1c30
drivers/net/team/team.c:1947
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&team->lock);
lock(&team->lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
2 locks held by syz-executor4/6391:
#0: (____ptrval____) (rtnl_mutex){+.+.}, at: rtnl_lock
net/core/rtnetlink.c:77 [inline]
#0: (____ptrval____) (rtnl_mutex){+.+.}, at: rtnetlink_rcv_msg+0x412/0xc30
net/core/rtnetlink.c:4662
#1: (____ptrval____) (&team->lock){+.+.}, at: team_add_slave+0xdb/0x1c30
drivers/net/team/team.c:1947
stack backtrace:
CPU: 1 PID: 6391 Comm: syz-executor4 Not tainted 4.18.0-rc7+ #176
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
print_deadlock_bug kernel/locking/lockdep.c:1765 [inline]
check_deadlock kernel/locking/lockdep.c:1809 [inline]
validate_chain kernel/locking/lockdep.c:2405 [inline]
__lock_acquire.cold.64+0x1fb/0x486 kernel/locking/lockdep.c:3435
lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
__mutex_lock_common kernel/locking/mutex.c:757 [inline]
__mutex_lock+0x176/0x1820 kernel/locking/mutex.c:894
mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:909
team_vlan_rx_add_vid+0x3b/0x1e0 drivers/net/team/team.c:1868
vlan_add_rx_filter_info+0x14a/0x1d0 net/8021q/vlan_core.c:210
__vlan_vid_add net/8021q/vlan_core.c:278 [inline]
vlan_vid_add+0x63e/0x9d0 net/8021q/vlan_core.c:308
vlan_device_event.cold.12+0x2a/0x2f net/8021q/vlan.c:381
notifier_call_chain+0x180/0x390 kernel/notifier.c:93
__raw_notifier_call_chain kernel/notifier.c:394 [inline]
raw_notifier_call_chain+0x2d/0x40 kernel/notifier.c:401
call_netdevice_notifiers_info+0x3f/0x90 net/core/dev.c:1735
call_netdevice_notifiers net/core/dev.c:1753 [inline]
dev_open+0x173/0x1b0 net/core/dev.c:1433
team_port_add drivers/net/team/team.c:1219 [inline]
team_add_slave+0xa8b/0x1c30 drivers/net/team/team.c:1948
do_set_master+0x1c9/0x220 net/core/rtnetlink.c:2248
do_setlink+0xba4/0x3e10 net/core/rtnetlink.c:2382
rtnl_setlink+0x2a9/0x400 net/core/rtnetlink.c:2636
rtnetlink_rcv_msg+0x46e/0xc30 net/core/rtnetlink.c:4665
netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2455
rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:4683
netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
netlink_unicast+0x5a0/0x760 net/netlink/af_netlink.c:1343
netlink_sendmsg+0xa18/0xfd0 net/netlink/af_netlink.c:1908
sock_sendmsg_nosec net/socket.c:642 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:652
___sys_sendmsg+0x7fd/0x930 net/socket.c:2126
__sys_sendmsg+0x11d/0x290 net/socket.c:2164
__do_sys_sendmsg net/socket.c:2173 [inline]
__se_sys_sendmsg net/socket.c:2171 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2171
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x456b29
Code: fd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9706bf8c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007f9706bf96d4 RCX: 0000000000456b29
RDX: 0000000000000000 RSI: 0000000020000240 RDI: 0000000000000004
RBP: 00000000009300a0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000004d3548 R14: 00000000004c8227 R15: 0000000000000000
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ 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