* [PATCH net-next 1/2] net/ncsi: Add NCSI Broadcom OEM command
From: Vijay Khemka @ 2018-10-05 19:01 UTC (permalink / raw)
To: Samuel Mendoza-Jonas, David S. Miller, netdev, linux-kernel
Cc: vijaykhemka, openbmc @ lists . ozlabs . org,
Justin . Lee1 @ Dell . com, joel @ jms . id . au,
linux-aspeed @ lists . ozlabs . org, Sai Dasari,
christian @ cmd . nu
This patch adds OEM Broadcom commands and response handling. It also
defines OEM Get MAC Address handler to get and configure the device.
ncsi_oem_gma_handler_bcm: This handler send NCSI broadcom command for
getting mac address.
ncsi_rsp_handler_oem_bcm: This handles response received for all
broadcom OEM commands.
ncsi_rsp_handler_oem_bcm_gma: This handles get mac address response and
set it to device.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
net/ncsi/Kconfig | 6 ++++
net/ncsi/internal.h | 8 +++++
net/ncsi/ncsi-manage.c | 70 +++++++++++++++++++++++++++++++++++++++++-
net/ncsi/ncsi-pkt.h | 8 +++++
net/ncsi/ncsi-rsp.c | 42 ++++++++++++++++++++++++-
5 files changed, 132 insertions(+), 2 deletions(-)
diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 08a8a6031fd7..7f2b46108a24 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig
@@ -10,3 +10,9 @@ config NET_NCSI
support. Enable this only if your system connects to a network
device via NCSI and the ethernet driver you're using supports
the protocol explicitly.
+config NCSI_OEM_CMD_GET_MAC
+ bool "Get NCSI OEM MAC Address"
+ depends on NET_NCSI
+ ---help---
+ This allows to get MAC address from NCSI firmware and set them back to
+ controller.
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 3d0a33b874f5..45883b32790e 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -71,6 +71,13 @@ enum {
/* OEM Vendor Manufacture ID */
#define NCSI_OEM_MFR_MLX_ID 0x8119
#define NCSI_OEM_MFR_BCM_ID 0x113d
+/* Broadcom specific OEM Command */
+#define NCSI_OEM_BCM_CMD_GMA 0x01 /* CMD ID for Get MAC */
+/* OEM Command payload lengths*/
+#define NCSI_OEM_BCM_CMD_GMA_LEN 12
+/* Mac address offset in OEM response */
+#define BCM_MAC_ADDR_OFFSET 28
+
struct ncsi_channel_version {
u32 version; /* Supported BCD encoded NCSI version */
@@ -240,6 +247,7 @@ enum {
ncsi_dev_state_probe_dp,
ncsi_dev_state_config_sp = 0x0301,
ncsi_dev_state_config_cis,
+ ncsi_dev_state_config_oem_gma,
ncsi_dev_state_config_clear_vids,
ncsi_dev_state_config_svf,
ncsi_dev_state_config_ev,
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 091284760d21..e5bfd9245b5d 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -635,6 +635,39 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
return 0;
}
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
+
+/* NCSI OEM Command APIs */
+static void ncsi_oem_gma_handler_bcm(struct ncsi_cmd_arg *nca)
+{
+ int ret = 0;
+ unsigned char data[NCSI_OEM_BCM_CMD_GMA_LEN];
+
+ nca->payload = NCSI_OEM_BCM_CMD_GMA_LEN;
+
+ memset(data, 0, NCSI_OEM_BCM_CMD_GMA_LEN);
+ *(unsigned int *)data = ntohl(NCSI_OEM_MFR_BCM_ID);
+ data[5] = NCSI_OEM_BCM_CMD_GMA;
+
+ nca->data = data;
+
+ ret = ncsi_xmit_cmd(nca);
+ if (ret)
+ netdev_err(nca->ndp->ndev.dev,
+ "NCSI: Failed to transmit cmd 0x%x during configure\n",
+ nca->type);
+}
+
+/* OEM Command handlers initialization */
+static struct ncsi_oem_gma_handler {
+ unsigned int mfr_id;
+ void (*handler)(struct ncsi_cmd_arg *nca);
+} ncsi_oem_gma_handlers[] = {
+ { NCSI_OEM_MFR_BCM_ID, ncsi_oem_gma_handler_bcm }
+};
+
+#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
+
static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
{
struct ncsi_dev *nd = &ndp->ndev;
@@ -643,9 +676,10 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
struct ncsi_channel *nc = ndp->active_channel;
struct ncsi_channel *hot_nc = NULL;
struct ncsi_cmd_arg nca;
+ struct ncsi_oem_gma_handler *nch = NULL;
unsigned char index;
unsigned long flags;
- int ret;
+ int ret, i;
nca.ndp = ndp;
nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
@@ -685,6 +719,40 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
goto error;
}
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
+ nd->state = ncsi_dev_state_config_oem_gma;
+ break;
+ case ncsi_dev_state_config_oem_gma:
+ nca.type = NCSI_PKT_CMD_OEM;
+ nca.package = np->id;
+ nca.channel = nc->id;
+ ndp->pending_req_num = 1;
+
+ /* Check for manufacturer id and Find the handler */
+ for (i = 0; i < ARRAY_SIZE(ncsi_oem_gma_handlers); i++) {
+ if (ncsi_oem_gma_handlers[i].mfr_id ==
+ nc->version.mf_id) {
+ if (ncsi_oem_gma_handlers[i].handler)
+ nch = &ncsi_oem_gma_handlers[i];
+ else
+ nch = NULL;
+
+ break;
+ }
+ }
+
+ if (!nch) {
+ netdev_err(ndp->ndev.dev, "No handler available for GMA with MFR-ID (0x%x)\n",
+ nc->version.mf_id);
+ nd->state = ncsi_dev_state_config_clear_vids;
+ schedule_work(&ndp->work);
+ break;
+ }
+
+ /* Get Mac address from NCSI device */
+ nch->handler(&nca);
+#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
+
nd->state = ncsi_dev_state_config_clear_vids;
break;
case ncsi_dev_state_config_clear_vids:
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 0f2087c8d42a..4d3f06be38bd 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -165,6 +165,14 @@ struct ncsi_rsp_oem_pkt {
unsigned char data[]; /* Payload data */
};
+/* Broadcom Response Data */
+struct ncsi_rsp_oem_bcm_pkt {
+ unsigned char ver; /* Payload Version */
+ unsigned char type; /* OEM Command type */
+ __be16 len; /* Payload Length */
+ unsigned char data[]; /* Cmd specific Data */
+};
+
/* Get Link Status */
struct ncsi_rsp_gls_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Response header */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index d66b34749027..bc20f7036579 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -596,12 +596,52 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
return 0;
}
+/* Response handler for Broadcom command Get Mac Address */
+static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
+{
+ struct ncsi_rsp_oem_pkt *rsp;
+ struct ncsi_dev_priv *ndp = nr->ndp;
+ struct net_device *ndev = ndp->ndev.dev;
+ int ret = 0;
+ const struct net_device_ops *ops = ndev->netdev_ops;
+ struct sockaddr saddr;
+
+ /* Get the response header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+
+ saddr.sa_family = ndev->type;
+ ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+ memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
+ /* Increase mac address by 1 for BMC's address */
+ saddr.sa_data[ETH_ALEN - 1]++;
+ ret = ops->ndo_set_mac_address(ndev, &saddr);
+ if (ret < 0)
+ netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
+
+ return ret;
+}
+
+/* Response handler for Broadcom card */
+static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
+{
+ struct ncsi_rsp_oem_pkt *rsp;
+ struct ncsi_rsp_oem_bcm_pkt *bcm;
+
+ /* Get the response header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+ bcm = (struct ncsi_rsp_oem_bcm_pkt *)(rsp->data);
+
+ if (bcm->type == NCSI_OEM_BCM_CMD_GMA)
+ return ncsi_rsp_handler_oem_bcm_gma(nr);
+ return 0;
+}
+
static struct ncsi_rsp_oem_handler {
unsigned int mfr_id;
int (*handler)(struct ncsi_request *nr);
} ncsi_rsp_oem_handlers[] = {
{ NCSI_OEM_MFR_MLX_ID, NULL },
- { NCSI_OEM_MFR_BCM_ID, NULL }
+ { NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm }
};
/* Response handler for OEM command */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 0/6] Fixes, minor changes & cleanups for the Unicast MAC VLAN table
From: David Miller @ 2018-10-05 19:02 UTC (permalink / raw)
To: salil.mehta
Cc: yisen.zhuang, lipeng321, mehta.salil, netdev, linux-kernel,
linuxarm
In-Reply-To: <20181005170329.6512-1-salil.mehta@huawei.com>
From: Salil Mehta <salil.mehta@huawei.com>
Date: Fri, 5 Oct 2018 18:03:23 +0100
> This patch-set presents necessary modifications, fixes & cleanups related
> to Unicast MAC Vlan Table to support revision 0x21 hardware.
Series applied.
^ permalink raw reply
* Re: [PATCH] bpf: fix building without CONFIG_INET
From: Song Liu @ 2018-10-05 19:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Alexei Starovoitov, Daniel Borkmann, David S . Miller,
John Fastabend, Martin KaFai Lau, makita.toshiaki,
Lawrence Brakmo, Andrey Ignatov, Jesper Dangaard Brouer,
Jakub Kicinski, Mathieu Xhonneux, dsahern, Networking, open list
In-Reply-To: <20181005161526.843924-1-arnd@arndb.de>
On Fri, Oct 5, 2018 at 9:18 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> The newly added TCP and UDP handling fails to link when CONFIG_INET
> is disabled:
>
> net/core/filter.o: In function `sk_lookup':
> filter.c:(.text+0x7ff8): undefined reference to `tcp_hashinfo'
> filter.c:(.text+0x7ffc): undefined reference to `tcp_hashinfo'
> filter.c:(.text+0x8020): undefined reference to `__inet_lookup_established'
> filter.c:(.text+0x8058): undefined reference to `__inet_lookup_listener'
> filter.c:(.text+0x8068): undefined reference to `udp_table'
> filter.c:(.text+0x8070): undefined reference to `udp_table'
> filter.c:(.text+0x808c): undefined reference to `__udp4_lib_lookup'
> net/core/filter.o: In function `bpf_sk_release':
> filter.c:(.text+0x82e8): undefined reference to `sock_gen_put'
>
> The compiler can optimize it out and avoid those references for
> the most part, but we are missing a few steps here:
>
> - sk_lookup() should always have been marked 'static', this also
> avoids a warning about a missing prototype when building with
> 'make W=1'.
> - The BPF_CALL_x() macro needs a little change to allow marking
> the unneeded BPF call as 'static' and having the compiler
> drop them.
> - The reference to the bpf_func_proto must be made conditional.
>
> Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> include/linux/filter.h | 2 +-
> net/core/filter.c | 18 +++++++++++-------
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 6791a0ac0139..d9ec9d908bbe 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -428,9 +428,9 @@ struct sock_reuseport;
> u64, __ur_3, u64, __ur_4, u64, __ur_5)
>
> #define BPF_CALL_x(x, name, ...) \
> + u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
> static __always_inline \
> u64 ____##name(__BPF_MAP(x, __BPF_DECL_ARGS, __BPF_V, __VA_ARGS__)); \
> - u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
> u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)) \
> { \
> return ____##name(__BPF_MAP(x,__BPF_CAST,__BPF_N,__VA_ARGS__));\
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 30c6b2d3ef16..dd5fe021f44c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4817,7 +4817,7 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
> };
> #endif /* CONFIG_IPV6_SEG6_BPF */
>
> -struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> +static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> struct sk_buff *skb, u8 family, u8 proto)
> {
> int dif = skb->dev->ifindex;
> @@ -4902,13 +4902,13 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> return (unsigned long) sk;
> }
>
> -BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
> +static BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
> struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
> {
> return bpf_sk_lookup(skb, tuple, len, IPPROTO_TCP, netns_id, flags);
> }
BPF_CALL_x() has static already (before this patch). We should not
need change that
for all the BPF_CALL_?(). Joe's version looks better to me.
Thanks,
Song
>
> -static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
> +static const __maybe_unused struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
> .func = bpf_sk_lookup_tcp,
> .gpl_only = false,
> .pkt_access = true,
> @@ -4920,13 +4920,13 @@ static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
> .arg5_type = ARG_ANYTHING,
> };
>
> -BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
> +static BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
> struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
> {
> return bpf_sk_lookup(skb, tuple, len, IPPROTO_UDP, netns_id, flags);
> }
>
> -static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
> +static const __maybe_unused struct bpf_func_proto bpf_sk_lookup_udp_proto = {
> .func = bpf_sk_lookup_udp,
> .gpl_only = false,
> .pkt_access = true,
> @@ -4938,14 +4938,14 @@ static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
> .arg5_type = ARG_ANYTHING,
> };
>
> -BPF_CALL_1(bpf_sk_release, struct sock *, sk)
> +static BPF_CALL_1(bpf_sk_release, struct sock *, sk)
> {
> if (!sock_flag(sk, SOCK_RCU_FREE))
> sock_gen_put(sk);
> return 0;
> }
>
> -static const struct bpf_func_proto bpf_sk_release_proto = {
> +static const __maybe_unused struct bpf_func_proto bpf_sk_release_proto = {
> .func = bpf_sk_release,
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> @@ -5158,12 +5158,14 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> case BPF_FUNC_skb_ancestor_cgroup_id:
> return &bpf_skb_ancestor_cgroup_id_proto;
> #endif
> +#ifdef CONFIG_INET
> case BPF_FUNC_sk_lookup_tcp:
> return &bpf_sk_lookup_tcp_proto;
> case BPF_FUNC_sk_lookup_udp:
> return &bpf_sk_lookup_udp_proto;
> case BPF_FUNC_sk_release:
> return &bpf_sk_release_proto;
> +#endif
> default:
> return bpf_base_func_proto(func_id);
> }
> @@ -5264,12 +5266,14 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_sk_redirect_hash_proto;
> case BPF_FUNC_get_local_storage:
> return &bpf_get_local_storage_proto;
> +#ifdef CONFIG_INET
> case BPF_FUNC_sk_lookup_tcp:
> return &bpf_sk_lookup_tcp_proto;
> case BPF_FUNC_sk_lookup_udp:
> return &bpf_sk_lookup_udp_proto;
> case BPF_FUNC_sk_release:
> return &bpf_sk_release_proto;
> +#endif
> default:
> return bpf_base_func_proto(func_id);
> }
> --
> 2.18.0
>
^ permalink raw reply
* Re: [PATCH] bpf: fix building without CONFIG_INET
From: Daniel Borkmann @ 2018-10-05 19:07 UTC (permalink / raw)
To: Song Liu, Arnd Bergmann
Cc: Alexei Starovoitov, David S . Miller, John Fastabend,
Martin KaFai Lau, makita.toshiaki, Lawrence Brakmo,
Andrey Ignatov, Jesper Dangaard Brouer, Jakub Kicinski,
Mathieu Xhonneux, dsahern, Networking, open list
In-Reply-To: <CAPhsuW7pg9Wp5am3Kc1t3UoAG1=EYzUeq8E2k7Pmv8yGu9JaEQ@mail.gmail.com>
On 10/05/2018 09:02 PM, Song Liu wrote:
[...]
> BPF_CALL_x() has static already (before this patch). We should not
> need change that
> for all the BPF_CALL_?(). Joe's version looks better to me.
My preference as well, thanks!
^ permalink raw reply
* Re: [PATCH] ath10k: htt_rx: Fix signedness bug in ath10k_update_per_peer_tx_stats
From: Ben Greear @ 2018-10-05 19:09 UTC (permalink / raw)
To: Gustavo A. R. Silva, Kalle Valo, David S. Miller
Cc: ath10k, linux-wireless, netdev, linux-kernel
In-Reply-To: <20181005184245.GA11700@embeddedor.com>
On 10/05/2018 11:42 AM, Gustavo A. R. Silva wrote:
> Currently, the error handling for the call to function
> ath10k_get_legacy_rate_idx() doesn't work because
> *rate_idx* is of type u8 (8 bits, unsigned), which
> makes it impossible for it to hold a value less
> than 0.
>
> Fix this by changing the type of variable *rate_idx*
> to s8 (8 bits, signed).
There are more than 127 rates, are you sure this is doing
what you want?
Thanks,
Ben
>
> Addresses-Coverity-ID: 1473914 ("Unsigned compared against 0")
> Fixes: 0189dbd71cbd ("ath10k: get the legacy rate index to update the txrate table")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/net/wireless/ath/ath10k/htt_rx.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> index f240525..edd0e74 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -2753,7 +2753,8 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
> struct ath10k_per_peer_tx_stats *peer_stats)
> {
> struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> - u8 rate = 0, rate_idx = 0, sgi;
> + u8 rate = 0, sgi;
> + s8 rate_idx = 0;
> struct rate_info txrate;
>
> lockdep_assert_held(&ar->data_lock);
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH] ath10k: htt_rx: Fix signedness bug in ath10k_update_per_peer_tx_stats
From: Gustavo A. R. Silva @ 2018-10-05 19:14 UTC (permalink / raw)
To: Ben Greear, Kalle Valo, David S. Miller
Cc: ath10k, linux-wireless, netdev, linux-kernel
In-Reply-To: <3a55bec6-d20d-f500-e741-b228a86b7117@candelatech.com>
On 10/5/18 9:09 PM, Ben Greear wrote:
> On 10/05/2018 11:42 AM, Gustavo A. R. Silva wrote:
>> Currently, the error handling for the call to function
>> ath10k_get_legacy_rate_idx() doesn't work because
>> *rate_idx* is of type u8 (8 bits, unsigned), which
>> makes it impossible for it to hold a value less
>> than 0.
>>
>> Fix this by changing the type of variable *rate_idx*
>> to s8 (8 bits, signed).
>
> There are more than 127 rates, are you sure this is doing
> what you want?
>
Based on the following function, rate_idx can only hold values from 0 to 11
static inline int ath10k_get_legacy_rate_idx(struct ath10k *ar, u8 rate)
{
static const u8 legacy_rates[] = {1, 2, 5, 11, 6, 9, 12,
18, 24, 36, 48, 54};
int i;
for (i = 0; i < ARRAY_SIZE(legacy_rates); i++) {
if (rate == legacy_rates[i])
return i;
}
ath10k_warn(ar, "Invalid legacy rate %hhd peer stats", rate);
return -EINVAL;
}
Thanks
--
Gustavo
> Thanks,
> Ben
>
>>
>> Addresses-Coverity-ID: 1473914 ("Unsigned compared against 0")
>> Fixes: 0189dbd71cbd ("ath10k: get the legacy rate index to update the txrate table")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>> drivers/net/wireless/ath/ath10k/htt_rx.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
>> index f240525..edd0e74 100644
>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>> @@ -2753,7 +2753,8 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
>> struct ath10k_per_peer_tx_stats *peer_stats)
>> {
>> struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
>> - u8 rate = 0, rate_idx = 0, sgi;
>> + u8 rate = 0, sgi;
>> + s8 rate_idx = 0;
>> struct rate_info txrate;
>>
>> lockdep_assert_held(&ar->data_lock);
>>
>
>
^ permalink raw reply
* Re: [PATCH] ath10k: htt_rx: Fix signedness bug in ath10k_update_per_peer_tx_stats
From: Gustavo A. R. Silva @ 2018-10-05 19:15 UTC (permalink / raw)
To: Ben Greear, Kalle Valo, David S. Miller
Cc: ath10k-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <dd75a8aa-b87c-e325-cc1d-a0efe3664a32-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
On 10/5/18 9:14 PM, Gustavo A. R. Silva wrote:
>
>
> On 10/5/18 9:09 PM, Ben Greear wrote:
>> On 10/05/2018 11:42 AM, Gustavo A. R. Silva wrote:
>>> Currently, the error handling for the call to function
>>> ath10k_get_legacy_rate_idx() doesn't work because
>>> *rate_idx* is of type u8 (8 bits, unsigned), which
>>> makes it impossible for it to hold a value less
>>> than 0.
>>>
>>> Fix this by changing the type of variable *rate_idx*
>>> to s8 (8 bits, signed).
>>
>> There are more than 127 rates, are you sure this is doing
>> what you want?
>>
>
> Based on the following function, rate_idx can only hold values from 0 to 11
>
... and of course -EINVAL too
> static inline int ath10k_get_legacy_rate_idx(struct ath10k *ar, u8 rate)
> {
> static const u8 legacy_rates[] = {1, 2, 5, 11, 6, 9, 12,
> 18, 24, 36, 48, 54};
> int i;
>
> for (i = 0; i < ARRAY_SIZE(legacy_rates); i++) {
> if (rate == legacy_rates[i])
> return i;
> }
>
> ath10k_warn(ar, "Invalid legacy rate %hhd peer stats", rate);
> return -EINVAL;
> }
>
> Thanks
> --
> Gustavo
>
>> Thanks,
>> Ben
>>
>>>
>>> Addresses-Coverity-ID: 1473914 ("Unsigned compared against 0")
>>> Fixes: 0189dbd71cbd ("ath10k: get the legacy rate index to update the txrate table")
>>> Signed-off-by: Gustavo A. R. Silva <gustavo-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
>>> ---
>>> drivers/net/wireless/ath/ath10k/htt_rx.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> index f240525..edd0e74 100644
>>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> @@ -2753,7 +2753,8 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
>>> struct ath10k_per_peer_tx_stats *peer_stats)
>>> {
>>> struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
>>> - u8 rate = 0, rate_idx = 0, sgi;
>>> + u8 rate = 0, sgi;
>>> + s8 rate_idx = 0;
>>> struct rate_info txrate;
>>>
>>> lockdep_assert_held(&ar->data_lock);
>>>
>>
>>
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6
From: Eric Dumazet @ 2018-10-05 12:17 UTC (permalink / raw)
To: David Miller, dsahern; +Cc: netdev, weiwan, sd, xiyou.wangcong, dsahern
In-Reply-To: <20181004.215507.1973705600397352485.davem@davemloft.net>
On 10/04/2018 09:55 PM, David Miller wrote:
> From: David Ahern <dsahern@kernel.org>
> Date: Thu, 4 Oct 2018 20:07:50 -0700
>
>> From: David Ahern <dsahern@gmail.com>
>>
>> As part of the IPv6 fib info refactoring, the intent was to make metrics
>> handling for ipv6 identical to ipv4. One oversight in ip6_dst_destroy
>> led to confusion and a couple of incomplete attempts at finding and
>> fixing the resulting memory leak which was ultimately resolved by
>> ce7ea4af0838 ("ipv6: fix memory leak on dst->_metrics").
>>
>> Refactor metrics hanlding make the code really identical for v4 and v6,
>> and add a few test cases.
>
> Looks nice, series applied, thanks David.
>
Does not look well tested and reviewed to me.
^ permalink raw reply
* Re: [PATCH net-next,v2] IPv6 ifstats separation
From: Stephen Suryaputra @ 2018-10-05 13:00 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <cdf02f85-57ea-3371-cec3-ec155964f4d0@gmail.com>
On Thu, Oct 4, 2018 at 4:42 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> How have you decided some counters can be 'slow' and other 'fast' ?
>
> I can tell you I see many ultra-fast candidates in your 'slow' list :/
Based on what others have categorized based on what's in the code and
IMHO they make sense:
enum
{
IPSTATS_MIB_NUM = 0,
/* frequently written fields in fast path, kept in same cache line */
IPSTATS_MIB_INPKTS, /* InReceives */
IPSTATS_MIB_INOCTETS, /* InOctets */
IPSTATS_MIB_INDELIVERS, /* InDelivers */
IPSTATS_MIB_OUTFORWDATAGRAMS, /* OutForwDatagrams */
IPSTATS_MIB_OUTPKTS, /* OutRequests */
IPSTATS_MIB_OUTOCTETS, /* OutOctets */
/* other fields */
IPSTATS_MIB_INHDRERRORS, /* InHdrErrors */
...
__IPSTATS_MIB_MAX
};
>
> Also think about DDOS.
>
> After your patch, all these 'wrong packets' will incur an expensive
> operation on a shared and highly contented cache line,
> effectively making the attack easier to conduct.
>
I agree about it is becoming more expensive to hit the slow counters
due to the check whether they are enabled or not.
Do you think it should just be enabled always? Then the cost to hit
the slow counters is the same as what it is right now.
Stephen.
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: Consolidate metrics handling for ipv4 and ipv6
From: Eric Dumazet @ 2018-10-05 13:08 UTC (permalink / raw)
To: Eric Dumazet, David Miller, dsahern
Cc: netdev, weiwan, sd, xiyou.wangcong, dsahern
In-Reply-To: <2b4d849f-9bd3-28aa-7a1c-ad61ab584041@gmail.com>
On 10/05/2018 05:17 AM, Eric Dumazet wrote:
>
>
> On 10/04/2018 09:55 PM, David Miller wrote:
>> From: David Ahern <dsahern@kernel.org>
>> Date: Thu, 4 Oct 2018 20:07:50 -0700
>>
>>> From: David Ahern <dsahern@gmail.com>
>>>
>>> As part of the IPv6 fib info refactoring, the intent was to make metrics
>>> handling for ipv6 identical to ipv4. One oversight in ip6_dst_destroy
>>> led to confusion and a couple of incomplete attempts at finding and
>>> fixing the resulting memory leak which was ultimately resolved by
>>> ce7ea4af0838 ("ipv6: fix memory leak on dst->_metrics").
>>>
>>> Refactor metrics hanlding make the code really identical for v4 and v6,
>>> and add a few test cases.
>>
>> Looks nice, series applied, thanks David.
>>
>
> Does not look well tested and reviewed to me.
For some reason I have not received the patch series in my inbox, I only got
your "series applied" message.
Commit 767a2217533fed6 ("net: common metrics init helper for FIB entries")
is not correct because we need to better deal with error paths.
I will submit this more formally when I can reach my workstation in a few minutes :
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 6c1d817151cae45421dc976c5ea082b4115650be..74d97addf1af20dda0c2b6a2018e88696f9f7d5a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2976,6 +2976,8 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
rt->fib6_metrics = ip_fib_metrics_init(net, cfg->fc_mx, cfg->fc_mx_len);
if (IS_ERR(rt->fib6_metrics)) {
err = PTR_ERR(rt->fib6_metrics);
+ /* Do not leave garbage there. */
+ rt->fib6_metrics = (struct dst_metrics *)&dst_default_metrics;
goto out;
}
^ permalink raw reply related
* [PATCH net-next] bnxt_en: Remove unnecessary unsigned integer comparison and initialize variable
From: Gustavo A. R. Silva @ 2018-10-05 20:12 UTC (permalink / raw)
To: Michael Chan, David S. Miller; +Cc: netdev, linux-kernel, Gustavo A. R. Silva
There is no need to compare *val.vu32* with < 0 because
such variable is of type u32 (32 bits, unsigned), making it
impossible to hold a negative value. Fix this by removing
such comparison.
Also, initialize variable *max_val* to -1, just in case
it is not initialized to either BNXT_MSIX_VEC_MAX or
BNXT_MSIX_VEC_MIN_MAX before using it in a comparison
with val.vu32 at line 159:
if (val.vu32 > max_val)
Addresses-Coverity-ID: 1473915 ("Unsigned compared against 0")
Addresses-Coverity-ID: 1473920 ("Uninitialized scalar variable")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 8a10e01..140dbd6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -148,7 +148,7 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
union devlink_param_value val,
struct netlink_ext_ack *extack)
{
- int max_val;
+ int max_val = -1;
if (id == DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX)
max_val = BNXT_MSIX_VEC_MAX;
@@ -156,7 +156,7 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
if (id == DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN)
max_val = BNXT_MSIX_VEC_MIN_MAX;
- if (val.vu32 < 0 || val.vu32 > max_val) {
+ if (val.vu32 > max_val) {
NL_SET_ERR_MSG_MOD(extack, "MSIX value is exceeding the range");
return -EINVAL;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next,v2] IPv6 ifstats separation
From: Eric Dumazet @ 2018-10-05 13:13 UTC (permalink / raw)
To: Stephen Suryaputra, eric.dumazet; +Cc: netdev
In-Reply-To: <CAHapkUhWNyS0SGQmHh11SNKKtP7c2V9ct6R-9EZO01QXdLE1ug@mail.gmail.com>
On 10/05/2018 06:00 AM, Stephen Suryaputra wrote:
> On Thu, Oct 4, 2018 at 4:42 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> How have you decided some counters can be 'slow' and other 'fast' ?
>>
>> I can tell you I see many ultra-fast candidates in your 'slow' list :/
>
> Based on what others have categorized based on what's in the code and
> IMHO they make sense:
Well, you better test, because you missed a few counters that are hit hard in the fast
path for normal (non DDOS) packets.
>
> enum
> {
> IPSTATS_MIB_NUM = 0,
> /* frequently written fields in fast path, kept in same cache line */
> IPSTATS_MIB_INPKTS, /* InReceives */
> IPSTATS_MIB_INOCTETS, /* InOctets */
> IPSTATS_MIB_INDELIVERS, /* InDelivers */
> IPSTATS_MIB_OUTFORWDATAGRAMS, /* OutForwDatagrams */
> IPSTATS_MIB_OUTPKTS, /* OutRequests */
> IPSTATS_MIB_OUTOCTETS, /* OutOctets */
> /* other fields */
> IPSTATS_MIB_INHDRERRORS, /* InHdrErrors */
> ...
> __IPSTATS_MIB_MAX
> };
>
>>
>> Also think about DDOS.
>>
>> After your patch, all these 'wrong packets' will incur an expensive
>> operation on a shared and highly contented cache line,
>> effectively making the attack easier to conduct.
>>
>
> I agree about it is becoming more expensive to hit the slow counters
> due to the check whether they are enabled or not.
What do you mean ?
The real cost is having dozens of cpus updating the same cache lines if the SNMP counter
is an atomic instead of per-cpu counters.
Make sure to test this on a configuration with 16 (or more) RX queues,
and cpus handling NIC IRQS spread on multiple NUMA nodes.
^ permalink raw reply
* Re: [RFC PATCH] skb: Define NET_IP_ALIGN based on CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
From: Will Deacon @ 2018-10-05 13:16 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Ben Hutchings, Russell King, Catalin Marinas,
<netdev@vger.kernel.org>, linux-kernel, linux-s390,
Ben Dooks, linux-arm-kernel
In-Reply-To: <CAKv+Gu9MBJ0w+23XMg+w_EYEf0Hx8dkW-w-rf4Bzu_c3GN_YiQ@mail.gmail.com>
On Thu, Oct 04, 2018 at 07:43:59PM +0200, Ard Biesheuvel wrote:
> (+ Arnd, Russell, Catalin, Will)
>
> On 4 October 2018 at 19:36, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote:
> > NET_IP_ALIGN is supposed to be defined as 0 if DMA writes to an
> > unaligned buffer would be more expensive than CPU access to unaligned
> > header fields, and otherwise defined as 2.
> >
> > Currently only ppc64 and x86 configurations define it to be 0.
> > However several other architectures (conditionally) define
> > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, which seems to imply that
> > NET_IP_ALIGN should be 0.
> >
> > Remove the overriding definitions for ppc64 and x86 and define
> > NET_IP_ALIGN solely based on CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
> >
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
>
> While this makes sense for arm64, I don't think it is appropriate for
> ARM per se.
Agreed that this makes sense for arm64, and I'd be happy to take a patch
defining it as 0 there.
Will
^ permalink raw reply
* [PATCH] mac80211_hwsim: fix module init error paths for netlink
From: Alexey Khoroshilov @ 2018-10-05 20:22 UTC (permalink / raw)
To: Johannes Berg
Cc: Alexey Khoroshilov, Kalle Valo, linux-wireless, netdev,
linux-kernel, ldv-project
There is no unregister netlink notifier and family on error paths
in init_mac80211_hwsim(). Also there is an error path where
hwsim_class is not destroyed.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Fixes: 62759361eb49 ("mac80211-hwsim: Provide multicast event for HWSIM_CMD_NEW_RADIO")
---
drivers/net/wireless/mac80211_hwsim.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 07442ada6dd0..6532cb25a333 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3712,16 +3712,16 @@ static int __init init_mac80211_hwsim(void)
if (err)
goto out_unregister_pernet;
+ err = hwsim_init_netlink();
+ if (err)
+ goto out_unregister_driver;
+
hwsim_class = class_create(THIS_MODULE, "mac80211_hwsim");
if (IS_ERR(hwsim_class)) {
err = PTR_ERR(hwsim_class);
- goto out_unregister_driver;
+ goto out_exit_netlink;
}
- err = hwsim_init_netlink();
- if (err < 0)
- goto out_unregister_driver;
-
for (i = 0; i < radios; i++) {
struct hwsim_new_radio_params param = { 0 };
@@ -3827,6 +3827,8 @@ static int __init init_mac80211_hwsim(void)
free_netdev(hwsim_mon);
out_free_radios:
mac80211_hwsim_free();
+out_exit_netlink:
+ hwsim_exit_netlink();
out_unregister_driver:
platform_driver_unregister(&mac80211_hwsim_driver);
out_unregister_pernet:
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 2/2] netdev/phy: add MDIO bus multiplexer driven by a regmap
From: Andrew Lunn @ 2018-10-05 13:53 UTC (permalink / raw)
To: Pankaj Bansal
Cc: Florian Fainelli, netdev@vger.kernel.org, Alexandru Marginean
In-Reply-To: <HE1PR0402MB3323DB8B3C52F4C14D3BC46DF1EB0@HE1PR0402MB3323.eurprd04.prod.outlook.com>
> > > + ret = regmap_update_bits_check(s->regmap,
> > > + s->mux_reg,
> > > + s->mask,
> > > + desired_child,
> > > + &change);
> >
> > When getting the mask from DT, you use be32_to_cpup().
> > When testing the reg value against the mask, you use be32_to_cpup().
> > Here you do not use be32_to_cpup()?
>
> Can you please tell me for which variable you mean I should use be32_to_cpup?
> I use be32_to_cpup when reading device tree entries.
> After being read, their values are stored in structure. After that no need to do be32_to_cpup
desired_child is read from DT by the mdio-mux core.
I'm just wondering why any of this be32_to_cpup() is needed. Why not
just use of_property_read_u32()?
Andrew
^ permalink raw reply
* Re: [PATCH net-next RFC 0/8] udp and configurable gro
From: Paolo Abeni @ 2018-10-05 13:53 UTC (permalink / raw)
To: Willem de Bruijn, netdev; +Cc: steffen.klassert, davem, Willem de Bruijn
In-Reply-To: <20180914175941.213950-1-willemdebruijn.kernel@gmail.com>
Hi all,
On Fri, 2018-09-14 at 13:59 -0400, Willem de Bruijn wrote:
> This is a *very rough* draft. Mainly for discussion while we also
> look at another partially overlapping approach [1].
I'm wondering how we go on from this ? I'm fine with either approaches.
Also, I'm interested in [try to] enable GRO/GSO batching in the
forwarding path, as you outlined initially in the GSO series
submission. That should cover Steffen use-case, too, right?
Cheers,
Paolo
^ permalink raw reply
* Re: [PATCH net-next v3] wireless-drivers: rtnetlink wifi simulation device
From: Joel Fernandes @ 2018-10-05 21:05 UTC (permalink / raw)
To: Sergey Matyukevich
Cc: Cody Schuffelen, Johannes Berg, Kalle Valo, David S . Miller,
linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, kernel-team@android.com
In-Reply-To: <20181005143323.ezyd2x6x5ymlb7rg@bars>
On Fri, Oct 5, 2018 at 7:33 AM, Sergey Matyukevich
<sergey.matyukevich.os@quantenna.com> wrote:
> Hi Cody,
>
>> drivers/net/wireless/Kconfig | 7 +
>> drivers/net/wireless/Makefile | 2 +
>> drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
>> 3 files changed, 627 insertions(+)
>> create mode 100644 drivers/net/wireless/virt_wifi.c
>
> I did a quick check of your patch using checkpatch kernel tool,
> here is a summary of its output:
>
> $ ./scripts/checkpatch.pl --strict test.patch
> ...
> total: 165 errors, 428 warnings, 9 checks, 634 lines checked
>
> Most part of those complaints is about either whitespaces or code
> idents. I am not sure whether this is a patch itself or email client.
> So could you please take a look and run checkpatch on your side.
>
Yeah, it could be his email client, weird though because if I pull the
patch from the kernel.org archive's mbox though, I don't get any
errors except the MAINTAINERS file thing:
wget https://lore.kernel.org/lkml/20181004195906.201895-1-schuffelen@google.com/raw
-O /tmp/tmp.patch
./scripts/checkpatch.pl --strict /tmp/tmp.patch
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#167:
new file mode 100644
total: 0 errors, 1 warnings, 0 checks, 634 lines checked
- Joel
^ permalink raw reply
* Re: [PATCH net-next v3] wireless-drivers: rtnetlink wifi simulation device
From: Joel Fernandes @ 2018-10-05 21:08 UTC (permalink / raw)
To: Sergey Matyukevich
Cc: Cody Schuffelen, Johannes Berg, Kalle Valo, David S . Miller,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org
In-Reply-To: <CAJWu+ooTRUxj2PAiUkgjTLb+VC9OoBpkj9-_ZoC-oCS3owc5Hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Oct 5, 2018 at 2:05 PM, Joel Fernandes <joelaf-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Oct 5, 2018 at 7:33 AM, Sergey Matyukevich
> <sergey.matyukevich.os-P/7pdk10T0iB+jHODAdFcQ@public.gmane.org> wrote:
>> Hi Cody,
>>
>>> drivers/net/wireless/Kconfig | 7 +
>>> drivers/net/wireless/Makefile | 2 +
>>> drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
>>> 3 files changed, 627 insertions(+)
>>> create mode 100644 drivers/net/wireless/virt_wifi.c
>>
>> I did a quick check of your patch using checkpatch kernel tool,
>> here is a summary of its output:
>>
>> $ ./scripts/checkpatch.pl --strict test.patch
>> ...
>> total: 165 errors, 428 warnings, 9 checks, 634 lines checked
>>
>> Most part of those complaints is about either whitespaces or code
>> idents. I am not sure whether this is a patch itself or email client.
>> So could you please take a look and run checkpatch on your side.
>>
>
> Yeah, it could be his email client, weird though because if I pull the
> patch from the kernel.org archive's mbox though, I don't get any
> errors except the MAINTAINERS file thing:
>
> wget https://lore.kernel.org/lkml/20181004195906.201895-1-schuffelen-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org/raw
> -O /tmp/tmp.patch
> ./scripts/checkpatch.pl --strict /tmp/tmp.patch
>
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #167:
> new file mode 100644
>
> total: 0 errors, 1 warnings, 0 checks, 634 lines checked
>
FWIW, the X-Mailer on the patch is: git-send-email 2.19.0.605.g01d371f741-goog
So I am guessing this is some issue with the way the patch was
generated, or something else. Cody, care to share the steps you used
to generate and send the patch?
- Joel
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Jann Horn @ 2018-10-05 14:12 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Network Development,
Linux API
In-Reply-To: <CACAyw99ar0Wokyg4jx8FPyr1Z=7Cf+=WVgF6sLL1FK9JtHROjw@mail.gmail.com>
On Fri, Oct 5, 2018 at 9:42 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
> >
> > If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> > check in here, right? I doubt it matters, but I don't really like
> > seeing something like this exposed to unprivileged userspace just
> > because you need it for kernel testing.
>
> That would mean all tests have to run as root / with CAP_SYS_ADMIN
> which isn't ideal.
This patch basically means that it becomes easier for a local user to
construct a BPF hash table that has all of its values stuffed into a
single hash bucket, correct? Which makes it easier to create a BPF
program that generates unusually large RCU stalls by performing ~40000
BPF map lookups, each of which has to walk through the entire linked
list of the hash map bucket? I dislike exposing something like that to
unprivileged userspace.
And if you want to run the whole BPF test suite with all its tests,
don't you already need root privileges? Or is this a different test
suite?
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-10-05 14:21 UTC (permalink / raw)
To: jannh; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CAG48ez3wwGgc40uAg5nq9dnC3qsAD04jQZTWtPi7jg_VDws-fg@mail.gmail.com>
On Fri, 5 Oct 2018 at 15:12, Jann Horn <jannh@google.com> wrote:
>
> On Fri, Oct 5, 2018 at 9:42 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
> > >
> > > If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> > > check in here, right? I doubt it matters, but I don't really like
> > > seeing something like this exposed to unprivileged userspace just
> > > because you need it for kernel testing.
> >
> > That would mean all tests have to run as root / with CAP_SYS_ADMIN
> > which isn't ideal.
>
> This patch basically means that it becomes easier for a local user to
> construct a BPF hash table that has all of its values stuffed into a
> single hash bucket, correct? Which makes it easier to create a BPF
> program that generates unusually large RCU stalls by performing ~40000
> BPF map lookups, each of which has to walk through the entire linked
> list of the hash map bucket? I dislike exposing something like that to
> unprivileged userspace.
That's a good point, for which I don't have an answer. You could argue that
this was the status quo until the seed was randomised, so it seems
like this hasn't been a worry so far. Should it be going forward?
> And if you want to run the whole BPF test suite with all its tests,
> don't you already need root privileges? Or is this a different test
> suite?
No, I'm thinking about third parties that want to test their own BPF.
If you enable unprivileged BPF you can use BPF_PROG_TEST_RUN to
test your programs without root, if I'm not mistaken.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
From: David Miller @ 2018-10-05 21:24 UTC (permalink / raw)
To: ben.dooks
Cc: netdev, David.Laight, oneukum, linux-usb, linux-kernel,
linux-kernel
In-Reply-To: <20181002165602.21033-1-ben.dooks@codethink.co.uk>
From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Tue, 2 Oct 2018 17:56:02 +0100
> - memcpy(skb->data, &tx_cmd_a, 4);
> + ptr = skb_push(skb, 8);
> + tx_cmd_a = cpu_to_le32(tx_cmd_a);
> + tx_cmd_b = cpu_to_le32(tx_cmd_b);
> + memcpy(ptr, &tx_cmd_a, 4);
> + memcpy(ptr+4, &tx_cmd_b, 4);
Even a memcpy() through a void pointer does not guarantee that gcc will
not emit word sized loads and stores.
You must use the get_unaligned()/put_unaligned() facilities to do this
properly.
I also agree that making a proper type and structure instead of using
a void pointer would be better.
^ permalink raw reply
* Re: [PATCH 0/3] bpf: allow zero-initialising hash map seed
From: Lorenz Bauer @ 2018-10-05 14:27 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Alexei Starovoitov, netdev, linux-api
In-Reply-To: <59ef80ab-4f28-5c75-c394-55fcfd9bc8ca@iogearbox.net>
On Mon, 1 Oct 2018 at 20:12, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/01/2018 12:45 PM, Lorenz Bauer wrote:
> > This patch set adds a new flag BPF_F_ZERO_SEED, which allows
> > forcing the seed used by hash maps to zero. This makes
> > it possible to write deterministic tests.
> >
> > Based on an off-list conversation with Alexei Starovoitov and
> > Daniel Borkmann.
> >
> > Lorenz Bauer (3):
> > bpf: allow zero-initializing hash map seed
> > tools: sync linux/bpf.h
> > tools: add selftest for BPF_F_ZERO_SEED
> >
> > include/uapi/linux/bpf.h | 2 +
> > kernel/bpf/hashtab.c | 8 ++-
> > tools/include/uapi/linux/bpf.h | 2 +
> > tools/testing/selftests/bpf/test_maps.c | 67 +++++++++++++++++++++----
> > 4 files changed, 66 insertions(+), 13 deletions(-)
> >
>
> Please respin with proper SoB for each patch and non-empty commit
> description.
What does SoB mean? Point taken about the empty commit message.
> I think patch 1 should also have a more elaborate
> commit description on the use case for BPF_F_ZERO_SEED, and I
This came out of the off-list discussion we had about map hash functions,
where Alexei expressed concern that your change to randomise the seed
might catch users off-guard. I personally don't have a use case, but decided
to tackle it since it seemed a simple-ish fix to get acquainted with
the code base.
Maybe this isn't needed after all?
> think also a better comment in the uapi header that this is only
> meant for testing and not production use.
Will do, if you decide that this is worth having in the first place.
>
> Thanks,
> Daniel
Lorenz
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Jann Horn @ 2018-10-05 14:27 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Network Development,
Linux API
In-Reply-To: <CACAyw9-JcORMGDb3wm-E4VYmjVmvWqB5vVh+=qTvXmpFWye_QA@mail.gmail.com>
On Fri, Oct 5, 2018 at 4:21 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
> On Fri, 5 Oct 2018 at 15:12, Jann Horn <jannh@google.com> wrote:
> > On Fri, Oct 5, 2018 at 9:42 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > On Tue, 2 Oct 2018 at 21:00, Jann Horn <jannh@google.com> wrote:
> > > > If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
> > > > check in here, right? I doubt it matters, but I don't really like
> > > > seeing something like this exposed to unprivileged userspace just
> > > > because you need it for kernel testing.
> > >
> > > That would mean all tests have to run as root / with CAP_SYS_ADMIN
> > > which isn't ideal.
> >
> > This patch basically means that it becomes easier for a local user to
> > construct a BPF hash table that has all of its values stuffed into a
> > single hash bucket, correct? Which makes it easier to create a BPF
> > program that generates unusually large RCU stalls by performing ~40000
> > BPF map lookups, each of which has to walk through the entire linked
> > list of the hash map bucket? I dislike exposing something like that to
> > unprivileged userspace.
>
> That's a good point, for which I don't have an answer. You could argue that
> this was the status quo until the seed was randomised, so it seems
> like this hasn't been a worry so far. Should it be going forward?
I don't think that local DoS bugs, or bugs that locally degrade
performance, are a big deal, but I also think that the kernel should
try to avoid having such issues.
> > And if you want to run the whole BPF test suite with all its tests,
> > don't you already need root privileges? Or is this a different test
> > suite?
>
> No, I'm thinking about third parties that want to test their own BPF.
Ah. That wasn't clear to me from your patch description.
Can you please describe exactly why something that is not a kernel
unit test needs deterministic BPF hash map behavior?
> If you enable unprivileged BPF you can use BPF_PROG_TEST_RUN to
> test your programs without root, if I'm not mistaken.
^ permalink raw reply
* Re: [PATCH 0/3] bpf: allow zero-initialising hash map seed
From: Jann Horn @ 2018-10-05 14:29 UTC (permalink / raw)
To: lmb; +Cc: Daniel Borkmann, Alexei Starovoitov, Network Development,
Linux API
In-Reply-To: <CACAyw99h58NX6tE1KrgoNEWwX4U8WW3JaRnJGJ0EGXEoeqeMDw@mail.gmail.com>
On Fri, Oct 5, 2018 at 4:27 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Mon, 1 Oct 2018 at 20:12, Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 10/01/2018 12:45 PM, Lorenz Bauer wrote:
> > > This patch set adds a new flag BPF_F_ZERO_SEED, which allows
> > > forcing the seed used by hash maps to zero. This makes
> > > it possible to write deterministic tests.
> > >
> > > Based on an off-list conversation with Alexei Starovoitov and
> > > Daniel Borkmann.
> > >
> > > Lorenz Bauer (3):
> > > bpf: allow zero-initializing hash map seed
> > > tools: sync linux/bpf.h
> > > tools: add selftest for BPF_F_ZERO_SEED
> > >
> > > include/uapi/linux/bpf.h | 2 +
> > > kernel/bpf/hashtab.c | 8 ++-
> > > tools/include/uapi/linux/bpf.h | 2 +
> > > tools/testing/selftests/bpf/test_maps.c | 67 +++++++++++++++++++++----
> > > 4 files changed, 66 insertions(+), 13 deletions(-)
> > >
> >
> > Please respin with proper SoB for each patch and non-empty commit
> > description.
>
> What does SoB mean? Point taken about the empty commit message.
SoB is the Signed-off-by line. See
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
.
^ permalink raw reply
* Re: [PATCH] atm: nicstar: Replace spin_is_locked() with spin_trylock()
From: David Miller @ 2018-10-05 21:32 UTC (permalink / raw)
To: ldr709; +Cc: linux-kernel, paulmck, 3chas3, linux-atm-general, netdev
In-Reply-To: <20181004074657.17597-1-ldr709@gmail.com>
From: Lance Roy <ldr709@gmail.com>
Date: Thu, 4 Oct 2018 00:46:57 -0700
> ns_poll() used spin_is_locked() + spin_lock() to get achieve the same
> thing as a spin_trylock(), so simplify it by using that instead. This is
> also a step towards possibly removing spin_is_locked().
>
> Signed-off-by: Lance Roy <ldr709@gmail.com>
Applied to net-next.
^ 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