* Re: [bpf-next V6 PATCH 15/15] xdp: transition into using xdp_frame for ndo_xdp_xmit
From: Jesper Dangaard Brouer @ 2018-03-28 18:56 UTC (permalink / raw)
To: netdev, BjörnTöpel, magnus.karlsson
Cc: eugenia, Jason Wang, John Fastabend, Eran Ben Elisha,
Saeed Mahameed, galp, Daniel Borkmann, Alexei Starovoitov,
Tariq Toukan, brouer
In-Reply-To: <152214319113.9023.5993294930011281799.stgit@firesoul>
On Tue, 27 Mar 2018 11:33:11 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> Changing API ndo_xdp_xmit to take a struct xdp_frame instead of struct
> xdp_buff. This brings xdp_return_frame and ndp_xdp_xmit in sync.
Maintainers notice, that i40e just got XDP_REDIRECT xmit support in
net-next in commit d9314c474d4f ("i40e: add support for XDP_REDIRECT"),
which is not avail in bpf-next yet...
Thus, I guess I'll have to do a V7, with the need i40e changes, once
the trees are in sync.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 18:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <a5c8ff7c-8185-a3fd-89eb-45ddc099c03b@fb.com>
On Wed, 28 Mar 2018 11:19:34 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> On 3/28/18 11:10 AM, Steven Rostedt wrote:
> > On Wed, 28 Mar 2018 11:03:24 -0700
> > Alexei Starovoitov <ast@fb.com> wrote:
> >
> >> I can live with this overhead if Mathieu insists,
> >> but I prefer to keep it in 'struct tracepoint'.
> >>
> >> Thoughts?
> >
> > I'm fine with keeping it as is. We could probably use it for future
> > enhancements in perf and ftrace.
> >
> > Perhaps, we should just add a:
> >
> > #ifdef CONFIG_BPF_EVENTS
> >
> > Around the use cases of num_args.
>
> it sounds like a good idea, but implementation wise
> it will be ifdef CONFIG_BPF_EVENTS around u32 num_args;
> in struct tracepoint and similar double definition of
> DEFINE_TRACE_FN. One that uses num_args to init
> struct tracepoint and one that doesn't ?
> Feels like serious uglification of already macros heavy code.
> Also what it will address?
32bit bloat ;-)
But I agree, it's not worth uglifying it.
-- Steve
> cache hot/cold argument clearly doesn't apply.
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Jonathan Corbet @ 2018-03-28 18:39 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, luto, jlayton, carlos, viro,
dhowells, simo, eparis, serge, ebiederm, madzcar
In-Reply-To: <e284617ad667ad8f17958dd8babb87fe1b4d7205.1521179281.git.rgb@redhat.com>
On Fri, 16 Mar 2018 05:00:28 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:
> Implement the proc fs write to set the audit container ID of a process,
> emitting an AUDIT_CONTAINER record to document the event.
A little detail, but still...
> +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
> +{
> + struct task_struct *parent;
> + u64 pcontainerid, ccontainerid;
> +
> + /* Don't allow to set our own containerid */
> + if (current == task)
> + return -EPERM;
> + /* Don't allow the containerid to be unset */
> + if (!cid_valid(containerid))
> + return -EINVAL;
I went looking for cid_valid(), but it turns out you don't add it until
patch 5. That, I expect, will not be good for bisectability (or patch
review).
Thanks,
jon
^ permalink raw reply
* Re: [PATCH bpf-next RFC 2/2] Add MPLS label push/pop functions for EBPF
From: David Ahern @ 2018-03-28 18:23 UTC (permalink / raw)
To: Shrijeet Mukherjee, daniel; +Cc: netdev, roopa, ast
In-Reply-To: <20180328162715.113-2-shrijeetoss@gmail.com>
On 3/28/18 10:27 AM, Shrijeet Mukherjee wrote:
> diff --git a/include/net/mpls.h b/include/net/mpls.h
> index 2583dbc689b8..3a5b8c00823d 100644
> --- a/include/net/mpls.h
> +++ b/include/net/mpls.h
> @@ -24,14 +24,50 @@ struct mpls_shim_hdr {
> __be32 label_stack_entry;
> };
>
> +struct mpls_entry_decoded {
> + u32 label;
> + u8 ttl;
> + u8 tc;
> + u8 bos;
> +};
> +
> static inline bool eth_p_mpls(__be16 eth_type)
> {
> return eth_type == htons(ETH_P_MPLS_UC) ||
> eth_type == htons(ETH_P_MPLS_MC);
> }
>
> -static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
> +static inline struct mpls_shim_hdr *skb_mpls_hdr(const struct sk_buff *skb)
> {
> return (struct mpls_shim_hdr *)skb_network_header(skb);
> }
> +
> +static inline struct mpls_shim_hdr
> +mpls_entry_encode(u32 label, unsigned int ttl, unsigned int tc, bool bos)
> +{
> + struct mpls_shim_hdr result;
> +
> + result.label_stack_entry =
> + cpu_to_be32((label << MPLS_LS_LABEL_SHIFT)
> + | (tc << MPLS_LS_TC_SHIFT)
> + | (bos ? (1 << MPLS_LS_S_SHIFT) : 0)
> + | (ttl << MPLS_LS_TTL_SHIFT));
> +
> + return result;
> +}
> +
> +static inline
> +struct mpls_entry_decoded mpls_entry_decode(struct mpls_shim_hdr *hdr)
> +{
> + struct mpls_entry_decoded result;
> + unsigned int entry = be32_to_cpu(hdr->label_stack_entry);
> +
> + result.label = (entry & MPLS_LS_LABEL_MASK) >> MPLS_LS_LABEL_SHIFT;
> + result.ttl = (entry & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
> + result.tc = (entry & MPLS_LS_TC_MASK) >> MPLS_LS_TC_SHIFT;
> + result.bos = (entry & MPLS_LS_S_MASK) >> MPLS_LS_S_SHIFT;
> +
> + return result;
> +}
> +
> #endif
The above should be in patch 1, though without the mpls_hdr rename.
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 18b7c510c511..2278548e1f8f 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -439,6 +439,12 @@ union bpf_attr {
> * int bpf_skb_vlan_pop(skb)
> * Return: 0 on success or negative error
> *
> + * int bpf_skb_mpls_push(skb, num_lbls, lbls[])
> + * Return 0 on success or negative error
> + *
> + * int bpf_skb_mpls_pop(skb, num_lbls)
> + * Return number of popped labels, 0 is no-op, deliver packet to current dst
> + *
> * int bpf_skb_get_tunnel_key(skb, key, size, flags)
> * int bpf_skb_set_tunnel_key(skb, key, size, flags)
> * retrieve or populate tunnel metadata
> @@ -794,7 +800,10 @@ union bpf_attr {
> FN(msg_redirect_map), \
> FN(msg_apply_bytes), \
> FN(msg_cork_bytes), \
> - FN(msg_pull_data),
> + FN(msg_pull_data), \
> + FN(skb_mpls_push), \
> + FN(skb_mpls_pop),
> +
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 00f62fafc788..c96ae8ef423d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2522,7 +2522,7 @@ static int bpf_skb_adjust_net(struct sk_buff *skb, s32 len_diff)
> }
>
> BPF_CALL_4(bpf_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
> - u32, mode, u64, flags)
> + u32, mode, u64, flags)
drop the above whitespace change
> {
> if (unlikely(flags))
> return -EINVAL;
> @@ -2542,6 +2542,125 @@ static const struct bpf_func_proto bpf_skb_adjust_room_proto = {
> .arg4_type = ARG_ANYTHING,
> };
>
> +static int bpf_skb_mpls_net_grow(struct sk_buff *skb, int len_diff)
> +{
> + u32 off = skb_mac_header_len(skb); /*LL_RESERVED_SPACE ?? */
> + int ret;
> +
> + ret = skb_cow(skb, len_diff);
> + if (unlikely(ret < 0))
> + return ret;
> +
> + skb_set_inner_protocol(skb, skb->protocol);
> + skb_reset_inner_network_header(skb);
> +
> + ret = bpf_skb_generic_push(skb, off, len_diff);
> + if (unlikely(ret < 0))
> + return ret;
> +
> + skb_reset_mac_header(skb);
> + skb_set_network_header(skb, ETH_HLEN);
> + skb->protocol = eth_hdr(skb)->h_proto = htons(ETH_P_MPLS_UC);
> +
> + if (skb_is_gso(skb)) {
> +/* Due to header grow, MSS needs to be downgraded. */
> + skb_shinfo(skb)->gso_size -= len_diff;
> +/* Header must be checked, and gso_segs recomputed. */
> + skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
> + skb_shinfo(skb)->gso_segs = 0;
> + }
> +
> + bpf_compute_data_pointers(skb);
> + return 0;
> +}
The only thing MPLS specific is the protocol setting. Seems to me you
could leverage the existing bpf_skb_net_grow or even bpf_skb_adjust_net
and then set the protocols and headers in the caller of this function.
You already do something similar in bpf_skb_mpls_pop.
> +
> +BPF_CALL_2(bpf_skb_mpls_pop, struct sk_buff*, skb, u32, num_lbls)
> +{
> + u32 i = 0;
> + struct mpls_shim_hdr *hdr;
> + unsigned char *cursor;
> + struct mpls_entry_decoded dec;
> +
> + if (num_lbls == 0)
> + return 0;
> +
> + cursor = skb_network_header(skb);
> + do {
> + hdr = (struct mpls_shim_hdr *)cursor;
> + dec = mpls_entry_decode(hdr);
> + i++; cursor = cursor + sizeof(struct mpls_shim_hdr);
> + } while (dec.bos != 1 && i < num_lbls);
> +
> + bpf_push_mac_rcsum(skb);
> + skb_pull(skb, i * sizeof(struct mpls_shim_hdr));
> + skb_reset_network_header(skb);
> +
> + skb->protocol = eth_hdr(skb)->h_proto = htons(ETH_P_MPLS_UC);
If I pop 1 label and there are more of them, then the protocol does not
need to be adjusted -- it was already set to MPLS.
If I pop all of the labels, then the protocol is not MPLS, but whatever
the inner packet is -- IPv4 or IPv6 or other.
> + bpf_pull_mac_rcsum(skb);
> + bpf_compute_data_pointers(skb);
> +
> + return i;
> +}
> +
> +const struct bpf_func_proto bpf_skb_mpls_pop_proto = {
> + .func = bpf_skb_mpls_pop,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_ANYTHING,
> +};
> +EXPORT_SYMBOL_GPL(bpf_skb_mpls_pop_proto);
> +
> +BPF_CALL_3(bpf_skb_mpls_push, struct sk_buff*, skb,
> + __be32*, lbls, u32, num_lbls)
> +{
> + int ret, i;
> + unsigned int new_header_size = num_lbls * sizeof(__be32);
> + unsigned int ttl = 255;
> + struct dst_entry *dst = skb_dst(skb);
dst is not always set. e.g., any program on ingress prior to the layer 3
will not have it.
> + struct net_device *out_dev = dst->dev;
> + struct mpls_shim_hdr *hdr;
> + bool bos;
> +
> + /* Ensure there is enough space for the headers in the skb */
> + ret = bpf_skb_mpls_net_grow(skb, new_header_size);
> + if (ret < 0) {
> + trace_printk("COW was killed\n");
> + bpf_compute_data_pointers(skb);
> + return -ENOMEM;
> + }
> +
> + skb->dev = out_dev;
> +/* XXX this may need finesse to integrate with
> + * global TTL values for MPLS
> + */
> + if (dst->ops->family == AF_INET)
> + ttl = ip_hdr(skb)->ttl;
> + else if (dst->ops->family == AF_INET6)
> + ttl = ipv6_hdr(skb)->hop_limit;
> +
> + /* Push the new labels */
> + hdr = skb_mpls_hdr(skb);
> + bos = true;
> + for (i = num_lbls - 1; i >= 0; i--) {
> + hdr[i] = mpls_entry_encode(lbls[i], ttl, 0, bos);
> + bos = false;
> + }
> +
> + bpf_compute_data_pointers(skb);
> + return 0;
> +}
> +
> +const struct bpf_func_proto bpf_skb_mpls_push_proto = {
> + .func = bpf_skb_mpls_push,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_PTR_TO_MEM,
> + .arg3_type = ARG_CONST_SIZE,
> +};
> +EXPORT_SYMBOL_GPL(bpf_skb_mpls_push_proto);
> +
> static u32 __bpf_skb_min_len(const struct sk_buff *skb)
> {
> u32 min_len = skb_network_offset(skb);
> @@ -3019,6 +3138,8 @@ bool bpf_helper_changes_pkt_data(void *func)
> {
> if (func == bpf_skb_vlan_push ||
> func == bpf_skb_vlan_pop ||
> + func == bpf_skb_mpls_push ||
> + func == bpf_skb_mpls_pop ||
> func == bpf_skb_store_bytes ||
> func == bpf_skb_change_proto ||
> func == bpf_skb_change_head ||
> @@ -3682,6 +3803,10 @@ tc_cls_act_func_proto(enum bpf_func_id func_id)
> return &bpf_skb_vlan_push_proto;
> case BPF_FUNC_skb_vlan_pop:
> return &bpf_skb_vlan_pop_proto;
> + case BPF_FUNC_skb_mpls_push:
> + return &bpf_skb_mpls_push_proto;
> + case BPF_FUNC_skb_mpls_pop:
> + return &bpf_skb_mpls_pop_proto;
> case BPF_FUNC_skb_change_proto:
> return &bpf_skb_change_proto_proto;
> case BPF_FUNC_skb_change_type:
>
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 18:19 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328141045.1202afeb@gandalf.local.home>
On 3/28/18 11:10 AM, Steven Rostedt wrote:
> On Wed, 28 Mar 2018 11:03:24 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> I can live with this overhead if Mathieu insists,
>> but I prefer to keep it in 'struct tracepoint'.
>>
>> Thoughts?
>
> I'm fine with keeping it as is. We could probably use it for future
> enhancements in perf and ftrace.
>
> Perhaps, we should just add a:
>
> #ifdef CONFIG_BPF_EVENTS
>
> Around the use cases of num_args.
it sounds like a good idea, but implementation wise
it will be ifdef CONFIG_BPF_EVENTS around u32 num_args;
in struct tracepoint and similar double definition of
DEFINE_TRACE_FN. One that uses num_args to init
struct tracepoint and one that doesn't ?
Feels like serious uglification of already macros heavy code.
Also what it will address?
cache hot/cold argument clearly doesn't apply.
^ permalink raw reply
* Re: [PATCH bpf-next RFC 1/2] Organize MPLS headers
From: David Ahern @ 2018-03-28 18:20 UTC (permalink / raw)
To: Shrijeet Mukherjee, daniel; +Cc: netdev, roopa, ast
In-Reply-To: <20180328162715.113-1-shrijeetoss@gmail.com>
On 3/28/18 10:27 AM, Shrijeet Mukherjee wrote:
> From: Shrijeet Mukherjee <shrijeet@gmail.com>
>
> Prepare shared headers for new MPLS label push/pop EBPF helpers
>
> Signed-off-by: Shrijeet Mukherjee <shm@cumulusnetworks.com>
> ---
> include/net/mpls.h | 1 +
> net/core/filter.c | 2 ++
> net/mpls/af_mpls.c | 8 ++++----
> net/mpls/internal.h | 31 -------------------------------
> net/mpls/mpls_iptunnel.c | 2 +-
> net/openvswitch/actions.c | 12 ++++++------
> 6 files changed, 14 insertions(+), 42 deletions(-)
>
> diff --git a/include/net/mpls.h b/include/net/mpls.h
> index 1dbc669b770e..2583dbc689b8 100644
> --- a/include/net/mpls.h
> +++ b/include/net/mpls.h
> @@ -16,6 +16,7 @@
>
> #include <linux/if_ether.h>
> #include <linux/netdevice.h>
> +#include <uapi/linux/mpls.h>
drop the uapi; just include linux/mpls.h
>
> #define MPLS_HLEN 4
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index c86f03fd9ea5..00f62fafc788 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -29,6 +29,7 @@
> #include <linux/sock_diag.h>
> #include <linux/in.h>
> #include <linux/inet.h>
> +#include <linux/mpls.h>
> #include <linux/netdevice.h>
> #include <linux/if_packet.h>
> #include <linux/if_arp.h>
> @@ -56,6 +57,7 @@
> #include <net/sock_reuseport.h>
> #include <net/busy_poll.h>
> #include <net/tcp.h>
> +#include <net/mpls.h>
> #include <linux/bpf_trace.h>
Changes to this file are not needed with this patch.
>
> /**
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index d4a89a8be013..4e05391b77f0 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -170,7 +170,7 @@ static u32 mpls_multipath_hash(struct mpls_route *rt, struct sk_buff *skb)
> break;
>
> /* Read and decode the current label */
> - hdr = mpls_hdr(skb) + label_index;
> + hdr = skb_mpls_hdr(skb) + label_index;
No need to add skb_ prefix; no other protocol has the
skb_ prefix (ip_hdr, ipv6_hdr, eth_hdr, ...)
> diff --git a/net/mpls/internal.h b/net/mpls/internal.h
> index 768a302879b4..cd93cb201fed 100644
> --- a/net/mpls/internal.h
> +++ b/net/mpls/internal.h
> @@ -8,13 +8,6 @@
> */
> #define MAX_NEW_LABELS 30
>
> -struct mpls_entry_decoded {
> - u32 label;
> - u8 ttl;
> - u8 tc;
> - u8 bos;
> -};
> -
> struct mpls_pcpu_stats {
> struct mpls_link_stats stats;
> struct u64_stats_sync syncp;
> @@ -172,30 +165,6 @@ struct mpls_route { /* next hop label forwarding entry */
>
> #define endfor_nexthops(rt) }
>
> -static inline struct mpls_shim_hdr mpls_entry_encode(u32 label, unsigned ttl, unsigned tc, bool bos)
> -{
> - struct mpls_shim_hdr result;
> - result.label_stack_entry =
> - cpu_to_be32((label << MPLS_LS_LABEL_SHIFT) |
> - (tc << MPLS_LS_TC_SHIFT) |
> - (bos ? (1 << MPLS_LS_S_SHIFT) : 0) |
> - (ttl << MPLS_LS_TTL_SHIFT));
> - return result;
> -}
> -
> -static inline struct mpls_entry_decoded mpls_entry_decode(struct mpls_shim_hdr *hdr)
> -{
> - struct mpls_entry_decoded result;
> - unsigned entry = be32_to_cpu(hdr->label_stack_entry);
> -
> - result.label = (entry & MPLS_LS_LABEL_MASK) >> MPLS_LS_LABEL_SHIFT;
> - result.ttl = (entry & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
> - result.tc = (entry & MPLS_LS_TC_MASK) >> MPLS_LS_TC_SHIFT;
> - result.bos = (entry & MPLS_LS_S_MASK) >> MPLS_LS_S_SHIFT;
> -
> - return result;
> -}
> -
> static inline struct mpls_dev *mpls_dev_get(const struct net_device *dev)
> {
> return rcu_dereference_rtnl(dev->mpls_ptr);
With just this patch applied, MPLS can't compile since you are removing
these 2 functions.
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Keep ATU/VTU violation statistics
From: Florian Fainelli @ 2018-03-28 18:17 UTC (permalink / raw)
To: Andrew Lunn, David Miller; +Cc: netdev
In-Reply-To: <1522187980-23072-2-git-send-email-andrew@lunn.ch>
On 03/27/2018 02:59 PM, Andrew Lunn wrote:
> Count the numbers of various ATU and VTU violation statistics and
> return them as part of the ethtool -S statistics.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 50 ++++++++++++++++++++++++++++-----
> drivers/net/dsa/mv88e6xxx/chip.h | 13 ++++++---
> drivers/net/dsa/mv88e6xxx/global1_atu.c | 12 +++++---
> drivers/net/dsa/mv88e6xxx/global1_vtu.c | 8 ++++--
> drivers/net/dsa/mv88e6xxx/serdes.c | 15 ++++++----
> drivers/net/dsa/mv88e6xxx/serdes.h | 8 +++---
> 6 files changed, 78 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 9a5d786b4885..186021f98c5d 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -723,6 +723,24 @@ static int mv88e6320_stats_get_strings(struct mv88e6xxx_chip *chip,
> STATS_TYPE_BANK0 | STATS_TYPE_BANK1);
> }
>
> +static const uint8_t *mv88e6xxx_atu_vtu_stats_strings[] = {
Why not const char *?
> + "atu_member_violation",
> + "atu_miss_violation",
> + "atu_full_violation",
> + "vtu_member_violation",
> + "vtu_miss_violation",
> +};
> +
> +static void mv88e6xxx_atu_vtu_get_strings(uint8_t *data)
> +{
> + int i;
unsigned int i?
> +
> + for (i = 0; i < ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings); i++)
> + strlcpy(data + i * ETH_GSTRING_LEN,
> + mv88e6xxx_atu_vtu_stats_strings[i],
> + ETH_GSTRING_LEN);
> +}
> +
> static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
> uint8_t *data)
> {
> @@ -736,9 +754,12 @@ static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
>
> if (chip->info->ops->serdes_get_strings) {
> data += count * ETH_GSTRING_LEN;
> - chip->info->ops->serdes_get_strings(chip, port, data);
> + count = chip->info->ops->serdes_get_strings(chip, port, data);
> }
>
> + data += count * ETH_GSTRING_LEN;
> + mv88e6xxx_atu_vtu_get_strings(data);
> +
> mutex_unlock(&chip->reg_lock);
> }
>
> @@ -783,10 +804,13 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> if (chip->info->ops->serdes_get_sset_count)
> serdes_count = chip->info->ops->serdes_get_sset_count(chip,
> port);
> - if (serdes_count < 0)
> + if (serdes_count < 0) {
> count = serdes_count;
> - else
> - count += serdes_count;
> + goto out;
> + }
> + count += serdes_count;
> + count += ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings);
> +
> out:
> mutex_unlock(&chip->reg_lock);
>
> @@ -841,6 +865,16 @@ static int mv88e6390_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
> 0);
> }
>
> +static void mv88e6xxx_atu_vtu_get_stats(struct mv88e6xxx_chip *chip, int port,
> + uint64_t *data)
> +{
> + *data++ = chip->ports[port].atu_member_violation;
> + *data++ = chip->ports[port].atu_miss_violation;
> + *data++ = chip->ports[port].atu_full_violation;
> + *data++ = chip->ports[port].vtu_member_violation;
> + *data++ = chip->ports[port].vtu_miss_violation;
This looks fine, but I suppose you could just have an u64 pointer which
is initialized to point to atu_member_violation, and then just do
pointer arithmetics to iterate, this would avoid possibly missing that
function in case new ATU/VTU violations are handled in the future?
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Make VTU miss violations less spammy
From: Florian Fainelli @ 2018-03-28 18:11 UTC (permalink / raw)
To: Andrew Lunn, David Miller; +Cc: netdev
In-Reply-To: <1522187980-23072-3-git-send-email-andrew@lunn.ch>
On 03/27/2018 02:59 PM, Andrew Lunn wrote:
> VTU miss violations can happen under normal conditions. Don't spam the
> kernel log. The statistics counter will indicate it is happening, if
> anybody is interested.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/dsa/mv88e6xxx/global1_vtu.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> index 2cbaf946e7ed..e0f1b4f6e29f 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> @@ -547,11 +547,9 @@ static irqreturn_t mv88e6xxx_g1_vtu_prob_irq_thread_fn(int irq, void *dev_id)
> chip->ports[spid].vtu_member_violation++;
> }
>
> - if (val & MV88E6XXX_G1_VTU_OP_MISS_VIOLATION) {
> - dev_err_ratelimited(chip->dev, "VTU miss violation for vid %d, source port %d\n",
> - entry.vid, spid);
Why not keep it as a dev_dbg() message? Ideally we would want to keep
those message around when the port is enslaved to a bridge, and vlan
filtering is enabled. In other cases, I agree this is just spam with the
current error level.
> + if (val & MV88E6XXX_G1_VTU_OP_MISS_VIOLATION)
> chip->ports[spid].vtu_miss_violation++;
> - }
> +
> mutex_unlock(&chip->reg_lock);
>
> return IRQ_HANDLED;
>
--
Florian
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 18:10 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <80d7af37-10ef-28d7-f03f-27b4b5849cd1@fb.com>
On Wed, 28 Mar 2018 11:03:24 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> I can live with this overhead if Mathieu insists,
> but I prefer to keep it in 'struct tracepoint'.
>
> Thoughts?
I'm fine with keeping it as is. We could probably use it for future
enhancements in perf and ftrace.
Perhaps, we should just add a:
#ifdef CONFIG_BPF_EVENTS
Around the use cases of num_args.
-- Steve
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 18:03 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328133830.3d5d74d3@gandalf.local.home>
On 3/28/18 10:38 AM, Steven Rostedt wrote:
> On Wed, 28 Mar 2018 10:10:34 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>
>>> and have:
>>>
>>> u64 tp_offset = (u64)tp - (u64)_sdata;
>>>
>>> if (WARN_ON(tp_offset > UINT_MAX)
>>> return -EINVAL;
>>>
>>> btp->tp_offset = (u32)tp_offset;
>>
>> above math has to be build time constant, so warn_on likely
>> won't work.
>
> Right, it would require a BUILD_BUG_ON.
>
>> imo the whole thing is too fragile and obscure.
>> I suggest to compress this 8 bytes * num_of_tracepoints later.
>> Especially would be good to do it in one way for
>> bpf_raw_event_map, ftrace and other places.
>
> Fair enough. We can defer this shrinkage to another time. I only
> suggested it here over your concern for the added bloat.
Actually, I will take it back.
I think the current shape of the patch is better.
struct tracepoint is aligned to 32-bytes by linker
Though sizeof(struct tracepoint) == 48 it actually consumes 64 bytes
of memory.
(gdb) p (void*)&__tracepoint_sys_enter - (void*)&__tracepoint_sys_exit
$3 = 64
Adding num_args to 'struct tracepoint' makes it sizeof==56,
but it still takes 64-bytes in memory.
In this patch sizeof(struct bpf_raw_tp_map) == 16 and
(gdb) p (void*)&__bpf_trace_tp_map_sys_enter -
(void*)&__bpf_trace_tp_map_sys_exit
$3 = 16
so it consumes exactly the same 16-bytes.
If we add 'u32 num_args' to it, it will have
sizeof(struct bpf_raw_tp_map) == 24 and will consume 32-bytes
of memory.
So clearly adding num_args to struct tracepont gives zero additional
bloat vs before this patch set, whereas moving it to
struct bpf_raw_tp_map will add 16 * num_of_tracepoints bytes of memory.
I can live with this overhead if Mathieu insists,
but I prefer to keep it in 'struct tracepoint'.
Thoughts?
^ permalink raw reply
* Re: NFS mounts failing when keytab present on client
From: J. Bruce Fields @ 2018-03-28 18:04 UTC (permalink / raw)
To: Eric Biggers
Cc: Michael Young, Herbert Xu, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <20180328180334.GB3354@fieldses.org>
On Wed, Mar 28, 2018 at 02:03:34PM -0400, J. Bruce Fields wrote:
> Thanks, got it. Do you know how to find a commit id for that change?
> It's not entirely fair to blame the crypto change for what was really a
> latent nfs bug, but it might still be worth adding a Fixes: line just so
> people know where it needs backporting.
Whoops, I see you already did that. I should finish my inbox before
responding!
--b.
^ permalink raw reply
* Re: NFS mounts failing when keytab present on client
From: J. Bruce Fields @ 2018-03-28 18:03 UTC (permalink / raw)
To: Eric Biggers
Cc: Michael Young, Herbert Xu, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <20180328175051.GB185597@google.com>
On Wed, Mar 28, 2018 at 10:50:51AM -0700, Eric Biggers wrote:
> On Wed, Mar 28, 2018 at 11:46:28AM -0400, J. Bruce Fields wrote:
> > On Tue, Mar 27, 2018 at 03:29:50PM -0700, Eric Biggers wrote:
> > > Hi Michael,
> > >
> > > On Tue, Mar 27, 2018 at 11:06:14PM +0100, Michael Young wrote:
> > > > NFS mounts stopped working on one of my computers after a kernel update from
> > > > 4.15.3 to 4.15.4. I traced the problem to the commit
> > > > [46e8d06e423c4f35eac7a8b677b713b3ec9b0684] crypto: hash - prevent using
> > > > keyed hashes without setting key
> > > > and a later kernel with this patch reverted works normally.
> > > >
> > > > The problem seems to be related to kerberos as the mount fails when the
> > > > keytab is present, but works if I rename the keytab file. This is true even
> > > > though the mount is with sec=sys . The mount should also work with sec=krb5
> > > > but that also fails in the same way. When the mount fails there are errors
> > > > in dmesg like
> > > > [ 1232.522816] gss_marshal: gss_get_mic FAILED (851968)
> > > > [ 1232.522819] RPC: couldn't encode RPC header, exit EIO
> > > > [ 1232.522856] gss_marshal: gss_get_mic FAILED (851968)
> > > > [ 1232.522857] RPC: couldn't encode RPC header, exit EIO
> > > > [ 1232.522863] NFS: nfs4_discover_server_trunking unhandled error -5.
> > > > Exiting with error EIO
> > > > [ 1232.525039] gss_marshal: gss_get_mic FAILED (851968)
> > > > [ 1232.525042] RPC: couldn't encode RPC header, exit EIO
> > > >
> > > > Michael Young
> > >
> > > Thanks for the bug report. I think the error is coming from
> > > net/sunrpc/auth_gss/gss_krb5_crypto.c. There are two potential problems I see.
> > > The first one, which is definitely a bug, is that make_checksum_hmac_md5()
> > > allocates an HMAC transform and request, then does these crypto API calls:
> > >
> > > crypto_ahash_init()
> > > crypto_ahash_setkey()
> > > crypto_ahash_digest()
> > >
> > > This is wrong because it makes no sense to init() the HMAC request before the
> > > key has been set, and doubly so when it's calling digest() which is shorthand
> > > for init() + update() + final(). So I think it just needs to be removed. You
> > > can test the following patch:
> >
> > When was this introduced?
> >
> > 3b5cf20cf439 "sunrpc: Use skcipher and ahash/shash"
> > - probably not, assuming the above was still just as wrong with
> > crypto_hash_{init,setkey,digest} as it is with
> > crypto_ahash_{init,setkey,digest}
> >
> > So I'm guessing it was wrong from the start when it was added by
> > fffdaef2eb4a "gss_krb5: Add support for rc4-hmac encryption" 8 years
> > ago. Wonder why it took this long to notice? Did something else
> > change?
> >
> > --b.
>
> It was wrong from the start, but the crypto API only recently started enforcing
> that the key has to be set before init() or digest() is called. Before that the
> code was just doing unnecessary work, at least with the software HMAC
> implementation. Though, there are also hardware crypto drivers that implement
> HMAC-MD5, and it's not immediately obvious that they handle init() before
> setkey() as gracefully as the software implementation.
Thanks, got it. Do you know how to find a commit id for that change?
It's not entirely fair to blame the crypto change for what was really a
latent nfs bug, but it might still be worth adding a Fixes: line just so
people know where it needs backporting.
--b.
^ permalink raw reply
* [PATCH] sunrpc: remove incorrect HMAC request initialization
From: Eric Biggers @ 2018-03-28 17:57 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, J . Bruce Fields, Jeff Layton,
linux-nfs
Cc: netdev, linux-crypto, Herbert Xu, Michael Young, Eric Biggers,
stable
make_checksum_hmac_md5() is allocating an HMAC transform and doing
crypto API calls in the following order:
crypto_ahash_init()
crypto_ahash_setkey()
crypto_ahash_digest()
This is wrong because it makes no sense to init() the request before a
key has been set, given that the initial state depends on the key. And
digest() is short for init() + update() + final(), so in this case
there's no need to explicitly call init() at all.
Before commit 9fa68f620041 ("crypto: hash - prevent using keyed hashes
without setting key") the extra init() had no real effect, at least for
the software HMAC implementation. (There are also hardware drivers that
implement HMAC-MD5, and it's not immediately obvious how gracefully they
handle init() before setkey().) But now the crypto API detects this
incorrect initialization and returns -ENOKEY. This is breaking NFS
mounts in some cases.
Fix it by removing the incorrect call to crypto_ahash_init().
Reported-by: Michael Young <m.a.young@durham.ac.uk>
Fixes: 9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")
Fixes: fffdaef2eb4a ("gss_krb5: Add support for rc4-hmac encryption")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 12649c9fedab..8654494b4d0a 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -237,9 +237,6 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
- err = crypto_ahash_init(req);
- if (err)
- goto out;
err = crypto_ahash_setkey(hmac_md5, cksumkey, kctx->gk5e->keylength);
if (err)
goto out;
--
2.17.0.rc1.321.gba9d0f2565-goog
^ permalink raw reply related
* Re: NFS mounts failing when keytab present on client
From: Eric Biggers @ 2018-03-28 17:50 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Michael Young, Herbert Xu, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <20180328154628.GA3038@fieldses.org>
On Wed, Mar 28, 2018 at 11:46:28AM -0400, J. Bruce Fields wrote:
> On Tue, Mar 27, 2018 at 03:29:50PM -0700, Eric Biggers wrote:
> > Hi Michael,
> >
> > On Tue, Mar 27, 2018 at 11:06:14PM +0100, Michael Young wrote:
> > > NFS mounts stopped working on one of my computers after a kernel update from
> > > 4.15.3 to 4.15.4. I traced the problem to the commit
> > > [46e8d06e423c4f35eac7a8b677b713b3ec9b0684] crypto: hash - prevent using
> > > keyed hashes without setting key
> > > and a later kernel with this patch reverted works normally.
> > >
> > > The problem seems to be related to kerberos as the mount fails when the
> > > keytab is present, but works if I rename the keytab file. This is true even
> > > though the mount is with sec=sys . The mount should also work with sec=krb5
> > > but that also fails in the same way. When the mount fails there are errors
> > > in dmesg like
> > > [ 1232.522816] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522819] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522856] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522857] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522863] NFS: nfs4_discover_server_trunking unhandled error -5.
> > > Exiting with error EIO
> > > [ 1232.525039] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.525042] RPC: couldn't encode RPC header, exit EIO
> > >
> > > Michael Young
> >
> > Thanks for the bug report. I think the error is coming from
> > net/sunrpc/auth_gss/gss_krb5_crypto.c. There are two potential problems I see.
> > The first one, which is definitely a bug, is that make_checksum_hmac_md5()
> > allocates an HMAC transform and request, then does these crypto API calls:
> >
> > crypto_ahash_init()
> > crypto_ahash_setkey()
> > crypto_ahash_digest()
> >
> > This is wrong because it makes no sense to init() the HMAC request before the
> > key has been set, and doubly so when it's calling digest() which is shorthand
> > for init() + update() + final(). So I think it just needs to be removed. You
> > can test the following patch:
>
> When was this introduced?
>
> 3b5cf20cf439 "sunrpc: Use skcipher and ahash/shash"
> - probably not, assuming the above was still just as wrong with
> crypto_hash_{init,setkey,digest} as it is with
> crypto_ahash_{init,setkey,digest}
>
> So I'm guessing it was wrong from the start when it was added by
> fffdaef2eb4a "gss_krb5: Add support for rc4-hmac encryption" 8 years
> ago. Wonder why it took this long to notice? Did something else
> change?
>
> --b.
It was wrong from the start, but the crypto API only recently started enforcing
that the key has to be set before init() or digest() is called. Before that the
code was just doing unnecessary work, at least with the software HMAC
implementation. Though, there are also hardware crypto drivers that implement
HMAC-MD5, and it's not immediately obvious that they handle init() before
setkey() as gracefully as the software implementation.
Eric
^ permalink raw reply
* Re: NFS mounts failing when keytab present on client
From: Eric Biggers @ 2018-03-28 17:47 UTC (permalink / raw)
To: M A Young
Cc: Herbert Xu, J. Bruce Fields, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <alpine.LFD.2.21.1803280857200.2500@algedi.dur.ac.uk>
On Wed, Mar 28, 2018 at 09:00:14AM +0100, M A Young wrote:
> On Tue, 27 Mar 2018, Eric Biggers wrote:
>
> > Hi Michael,
> >
> > On Tue, Mar 27, 2018 at 11:06:14PM +0100, Michael Young wrote:
> > > NFS mounts stopped working on one of my computers after a kernel update from
> > > 4.15.3 to 4.15.4. I traced the problem to the commit
> > > [46e8d06e423c4f35eac7a8b677b713b3ec9b0684] crypto: hash - prevent using
> > > keyed hashes without setting key
> > > and a later kernel with this patch reverted works normally.
> > >
> > > The problem seems to be related to kerberos as the mount fails when the
> > > keytab is present, but works if I rename the keytab file. This is true even
> > > though the mount is with sec=sys . The mount should also work with sec=krb5
> > > but that also fails in the same way. When the mount fails there are errors
> > > in dmesg like
> > > [ 1232.522816] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522819] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522856] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522857] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522863] NFS: nfs4_discover_server_trunking unhandled error -5.
> > > Exiting with error EIO
> > > [ 1232.525039] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.525042] RPC: couldn't encode RPC header, exit EIO
> > >
> > > Michael Young
> >
> > Thanks for the bug report. I think the error is coming from
> > net/sunrpc/auth_gss/gss_krb5_crypto.c. There are two potential problems I see.
> > The first one, which is definitely a bug, is that make_checksum_hmac_md5()
> > allocates an HMAC transform and request, then does these crypto API calls:
> >
> > crypto_ahash_init()
> > crypto_ahash_setkey()
> > crypto_ahash_digest()
> >
> > This is wrong because it makes no sense to init() the HMAC request before the
> > key has been set, and doubly so when it's calling digest() which is shorthand
> > for init() + update() + final(). So I think it just needs to be removed. You
> > can test the following patch:
> >
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> > index 12649c9fedab..8654494b4d0a 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> > @@ -237,9 +237,6 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
> >
> > ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
> >
> > - err = crypto_ahash_init(req);
> > - if (err)
> > - goto out;
> > err = crypto_ahash_setkey(hmac_md5, cksumkey, kctx->gk5e->keylength);
> > if (err)
> > goto out;
> >
> > If that's not it, it's also possible that the error is coming from the
> > crypto_ahash_init() in make_checksum(). That can only happen if 'cksumkey' is
> > NULL and the hash algorithm is keyed, which implies a logical error as it
> > doesn't make sense to use a keyed hash algorithm without the key. The callers
> > do check kctx->gk5e->keyed_cksum which I'd hope would prevent this, though
> > perhaps kctx->cksum can be NULL.
> >
> > Eric
>
> The patch fixes the problem.
>
> Michael Young
Okay, thanks for testing! I'll send a formal patch.
Eric
^ permalink raw reply
* Re: [PATCH][next] wil6210: fix potential null dereference of ndev before null check
From: merez @ 2018-03-28 17:46 UTC (permalink / raw)
To: Colin King
Cc: Kalle Valo, linux-wireless, wil6210, netdev, kernel-janitors,
linux-kernel, linux-wireless-owner
In-Reply-To: <20180328174027.31551-1-colin.king@canonical.com>
On 2018-03-28 20:40, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer ndev is being dereferenced before it is being null checked,
> hence there is a potential null pointer deference. Fix this by only
> dereferencing ndev after it has been null checked
>
> Detected by CoverityScan, CID#1467010 ("Dereference before null check")
>
> Fixes: e00243fab84b ("wil6210: infrastructure for multiple virtual
> interfaces")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wireless/ath/wil6210/main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/main.c
> b/drivers/net/wireless/ath/wil6210/main.c
> index a4b413e8d55a..82aec6b06d09 100644
> --- a/drivers/net/wireless/ath/wil6210/main.c
> +++ b/drivers/net/wireless/ath/wil6210/main.c
> @@ -391,7 +391,7 @@ static void wil_fw_error_worker(struct work_struct
> *work)
> struct wil6210_priv *wil = container_of(work, struct wil6210_priv,
> fw_error_worker);
> struct net_device *ndev = wil->main_ndev;
> - struct wireless_dev *wdev = ndev->ieee80211_ptr;
> + struct wireless_dev *wdev;
>
> wil_dbg_misc(wil, "fw error worker\n");
>
> @@ -399,6 +399,7 @@ static void wil_fw_error_worker(struct work_struct
> *work)
> wil_info(wil, "No recovery - interface is down\n");
> return;
> }
> + wdev = ndev->ieee80211_ptr;
>
> /* increment @recovery_count if less then WIL6210_FW_RECOVERY_TO
> * passed since last recovery attempt
Reviewed-by: Maya Erez <merez@codeaurora.org>
--
Maya Erez
Qualcomm Israel, Inc. on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH bpf-next]: add sock_ops R/W access to ipv4 tos
From: Nikita Shirokov @ 2018-03-28 17:41 UTC (permalink / raw)
To: Daniel Borkmann, Lawrence Brakmo, ast@kernel.org,
netdev@vger.kernel.org
Cc: Kernel Team
In-Reply-To: <67c87527-6f6c-d5fb-5fe9-83c3aa1d000d@iogearbox.net>
>>On 03/26/2018 05:36 PM, Nikita V. Shirokov wrote:
>> bpf: Add sock_ops R/W access to ipv4 tos
>>
>> Sample usage for tos:
>>
>> bpf_getsockopt(skops, SOL_IP, IP_TOS, &v, sizeof(v))
>>
>> where skops is a pointer to the ctx (struct bpf_sock_ops).
>>
>> Signed-off-by: Nikita V. Shirokov <tehnerd@fb.com>
>> ---
>> net/core/filter.c | 35 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 00c711c..afd8255 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -3462,6 +3462,27 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>> ret = -EINVAL;
>> }
>> #ifdef CONFIG_INET
>> + } else if (level == SOL_IP) {
>> + if (optlen != sizeof(int) || sk->sk_family != AF_INET)
>> + return -EINVAL;
>> +
>> + val = *((int *)optval);
>> + /* Only some options are supported */
>> + switch (optname) {
>> + case IP_TOS:
>> + if (val < -1 || val > 0xff) {
>> + ret = -EINVAL;
>> + } else {
>> + struct inet_sock *inet = inet_sk(sk);
>> +
>> + if (val == -1)
>> + val = 0;
>> + inet->tos = val;
>
>Should this not have the exact same semantics given the helper resembles
>the normal setsockopt? do_ip_setsockopt() does the following when setting
>IP_TOS:
>
> case IP_TOS: /* This sets both TOS and Precedence */
> if (sk->sk_type == SOCK_STREAM) {
> val &= ~INET_ECN_MASK;
> val |= inet->tos & INET_ECN_MASK;
> }
> if (inet->tos != val) {
> inet->tos = val;
> sk->sk_priority = rt_tos2priority(val);
> sk_dst_reset(sk);
> }
> break;
>
>E.g. why we don't need to set sk->sk_priority as well or reset the dst
>entry here?
>
it feels like initially (w/ commit for IP_TOS in ip_sockglue.c) there were some usecase in mind
where reflection of tos to prio was needed + some policy based routing (thats why dst_reset).
but e.g. for ipv6 (IPV6_TCLASS, same as TOS but in ipv6 world) we do just this - set new tclass value
and call it the day. in my opinion this aproach is more flexible (e.g. we have separate
bpf_setsockopt for SOL_PRIORITY) as it did only what we want (i can imagine few usecases
where we want just to change TOS w/o changing priority)
however if you feel strong and want to reproduce the same behavior as in regular setsockopt, i can
totally make v2. please let me know
>> + }
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> #if IS_ENABLED(CONFIG_IPV6)
>> } else if (level == SOL_IPV6) {
>> if (optlen != sizeof(int) || sk->sk_family != AF_INET6)
>> @@ -3561,6 +3582,20 @@ BPF_CALL_5(bpf_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>> } else {
>> goto err_clear;
>> }
>> + } else if (level == SOL_IP) {
>> + struct inet_sock *inet = inet_sk(sk);
>> +
>> + if (optlen != sizeof(int) || sk->sk_family != AF_INET)
>> + goto err_clear;
>> +
>> + /* Only some options are supported */
>> + switch (optname) {
>> + case IP_TOS:
>> + *((int *)optval) = (int)inet->tos;
>
>This part is fine though, same as in do_ip_getsockopt().
>
>> + break;
>> + default:
>> + goto err_clear;
>> + }
>> #if IS_ENABLED(CONFIG_IPV6)
>> } else if (level == SOL_IPV6) {
>> struct ipv6_pinfo *np = inet6_sk(sk);
>>
^ permalink raw reply
* Re: [PATCH v7 bpf-next 07/10] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-28 17:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev,
kernel-team, linux-api
In-Reply-To: <20180328021105.4061744-8-ast@fb.com>
On Tue, 27 Mar 2018 19:11:02 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> Introduce BPF_PROG_TYPE_RAW_TRACEPOINT bpf program type to access
> kernel internal arguments of the tracepoints in their raw form.
>
> >From bpf program point of view the access to the arguments look like:
> struct bpf_raw_tracepoint_args {
> __u64 args[0];
> };
>
> int bpf_prog(struct bpf_raw_tracepoint_args *ctx)
> {
> // program can read args[N] where N depends on tracepoint
> // and statically verified at program load+attach time
> }
>
> kprobe+bpf infrastructure allows programs access function arguments.
> This feature allows programs access raw tracepoint arguments.
>
> Similar to proposed 'dynamic ftrace events' there are no abi guarantees
> to what the tracepoints arguments are and what their meaning is.
> The program needs to type cast args properly and use bpf_probe_read()
> helper to access struct fields when argument is a pointer.
>
> For every tracepoint __bpf_trace_##call function is prepared.
> In assembler it looks like:
> (gdb) disassemble __bpf_trace_xdp_exception
> Dump of assembler code for function __bpf_trace_xdp_exception:
> 0xffffffff81132080 <+0>: mov %ecx,%ecx
> 0xffffffff81132082 <+2>: jmpq 0xffffffff811231f0 <bpf_trace_run3>
>
> where
>
> TRACE_EVENT(xdp_exception,
> TP_PROTO(const struct net_device *dev,
> const struct bpf_prog *xdp, u32 act),
>
> The above assembler snippet is casting 32-bit 'act' field into 'u64'
> to pass into bpf_trace_run3(), while 'dev' and 'xdp' args are passed as-is.
> All of ~500 of __bpf_trace_*() functions are only 5-10 byte long
> and in total this approach adds 7k bytes to .text.
>
> This approach gives the lowest possible overhead
> while calling trace_xdp_exception() from kernel C code and
> transitioning into bpf land.
> Since tracepoint+bpf are used at speeds of 1M+ events per second
> this is valuable optimization.
>
> The new BPF_RAW_TRACEPOINT_OPEN sys_bpf command is introduced
> that returns anon_inode FD of 'bpf-raw-tracepoint' object.
>
> The user space looks like:
> // load bpf prog with BPF_PROG_TYPE_RAW_TRACEPOINT type
> prog_fd = bpf_prog_load(...);
> // receive anon_inode fd for given bpf_raw_tracepoint with prog attached
> raw_tp_fd = bpf_raw_tracepoint_open("xdp_exception", prog_fd);
>
> Ctrl-C of tracing daemon or cmdline tool that uses this feature
> will automatically detach bpf program, unload it and
> unregister tracepoint probe.
>
> On the kernel side the __bpf_raw_tp_map section of pointers to
> tracepoint definition and to __bpf_trace_*() probe function is used
> to find a tracepoint with "xdp_exception" name and
> corresponding __bpf_trace_xdp_exception() probe function
> which are passed to tracepoint_probe_register() to connect probe
> with tracepoint.
>
> Addition of bpf_raw_tracepoint doesn't interfere with ftrace and perf
> tracepoint mechanisms. perf_event_open() can be used in parallel
> on the same tracepoint.
> Multiple bpf_raw_tracepoint_open("xdp_exception", prog_fd) are permitted.
> Each with its own bpf program. The kernel will execute
> all tracepoint probes and all attached bpf programs.
>
> In the future bpf_raw_tracepoints can be extended with
> query/introspection logic.
>
> __bpf_raw_tp_map section logic was contributed by Steven Rostedt
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
Just an FYI, I applied all the patches up to and including this one
(made sure BPF_EVENTS was enabled in my config this time), built and
booted the kernel and ran a bunch of tests (not my full suite, but
enough).
It didn't affect any other tracing features that I can see.
-- Steve
^ permalink raw reply
* [PATCH][next] wil6210: fix potential null dereference of ndev before null check
From: Colin King @ 2018-03-28 17:40 UTC (permalink / raw)
To: Maya Erez, Kalle Valo, linux-wireless, wil6210, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The pointer ndev is being dereferenced before it is being null checked,
hence there is a potential null pointer deference. Fix this by only
dereferencing ndev after it has been null checked
Detected by CoverityScan, CID#1467010 ("Dereference before null check")
Fixes: e00243fab84b ("wil6210: infrastructure for multiple virtual interfaces")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/ath/wil6210/main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wil6210/main.c b/drivers/net/wireless/ath/wil6210/main.c
index a4b413e8d55a..82aec6b06d09 100644
--- a/drivers/net/wireless/ath/wil6210/main.c
+++ b/drivers/net/wireless/ath/wil6210/main.c
@@ -391,7 +391,7 @@ static void wil_fw_error_worker(struct work_struct *work)
struct wil6210_priv *wil = container_of(work, struct wil6210_priv,
fw_error_worker);
struct net_device *ndev = wil->main_ndev;
- struct wireless_dev *wdev = ndev->ieee80211_ptr;
+ struct wireless_dev *wdev;
wil_dbg_misc(wil, "fw error worker\n");
@@ -399,6 +399,7 @@ static void wil_fw_error_worker(struct work_struct *work)
wil_info(wil, "No recovery - interface is down\n");
return;
}
+ wdev = ndev->ieee80211_ptr;
/* increment @recovery_count if less then WIL6210_FW_RECOVERY_TO
* passed since last recovery attempt
--
2.15.1
^ permalink raw reply related
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 17:38 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <fdcb0bf0-f896-ffb3-b9de-0b17f5f70fc6@fb.com>
On Wed, 28 Mar 2018 10:10:34 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> > and have:
> >
> > u64 tp_offset = (u64)tp - (u64)_sdata;
> >
> > if (WARN_ON(tp_offset > UINT_MAX)
> > return -EINVAL;
> >
> > btp->tp_offset = (u32)tp_offset;
>
> above math has to be build time constant, so warn_on likely
> won't work.
Right, it would require a BUILD_BUG_ON.
> imo the whole thing is too fragile and obscure.
> I suggest to compress this 8 bytes * num_of_tracepoints later.
> Especially would be good to do it in one way for
> bpf_raw_event_map, ftrace and other places.
Fair enough. We can defer this shrinkage to another time. I only
suggested it here over your concern for the added bloat.
-- Steve
^ permalink raw reply
* Re: [PATCH v2 net-next] qed*: Utilize FW 8.33.11.0
From: Jason Gunthorpe @ 2018-03-28 17:32 UTC (permalink / raw)
To: Michal Kalderon
Cc: davem, netdev, linux-rdma, linux-scsi, Tomer Tayar,
Manish Rangankar, Ariel Elior
In-Reply-To: <1522226536-9812-1-git-send-email-Michal.Kalderon@cavium.com>
On Wed, Mar 28, 2018 at 11:42:16AM +0300, Michal Kalderon wrote:
> This FW contains several fixes and features
>
> RDMA Features
> - SRQ support
> - XRC support
> - Memory window support
> - RDMA low latency queue support
> - RDMA bonding support
>
> RDMA bug fixes
> - RDMA remote invalidate during retransmit fix
> - iWARP MPA connect interop issue with RTR fix
> - iWARP Legacy DPM support
> - Fix MPA reject flow
> - iWARP error handling
> - RQ WQE validation checks
>
> MISC
> - Fix some HSI types endianity
> - New Restriction: vlan insertion in core_tx_bd_data can't be set
> for LB packets
>
> ETH
> - HW QoS offload support
> - Fix vlan, dcb and sriov flow of VF sending a packet with
> inband VLAN tag instead of default VLAN
> - Allow GRE version 1 offloads in RX flow
> - Allow VXLAN steering
>
> iSCSI / FcoE
> - Fix bd availability checking flow
> - Support 256th sge proerly in iscsi/fcoe retransmit
> - Performance improvement
> - Fix handle iSCSI command arrival with AHS and with immediate
> - Fix ipv6 traffic class configuration
>
> DEBUG
> - Update debug utilities
>
> Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com>
> Signed-off-by: Tomer Tayar <Tomer.Tayar@cavium.com>
> Signed-off-by: Manish Rangankar <Manish.Rangankar@cavium.com>
> Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com>
> ---
> Changes from v1
> Remove version bump and qedr module version addition
> ---
> drivers/infiniband/hw/qedr/qedr_hsi_rdma.h | 4 +-
> drivers/infiniband/hw/qedr/verbs.c | 4 +-
> drivers/net/ethernet/qlogic/qed/qed_debug.c | 415 +++--
> drivers/net/ethernet/qlogic/qed/qed_dev.c | 4 +-
> drivers/net/ethernet/qlogic/qed/qed_hsi.h | 1892 ++++++++++----------
> .../net/ethernet/qlogic/qed/qed_init_fw_funcs.c | 103 +-
> drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 7 -
> drivers/net/ethernet/qlogic/qed/qed_l2.c | 2 +-
> drivers/net/ethernet/qlogic/qed/qed_ll2.c | 13 -
> include/linux/qed/common_hsi.h | 2 +-
> include/linux/qed/eth_common.h | 2 +-
> include/linux/qed/iscsi_common.h | 4 +-
> include/linux/qed/rdma_common.h | 2 +
> include/linux/qed/roce_common.h | 3 +
> 14 files changed, 1310 insertions(+), 1147 deletions(-)
For drivers/infiniband:
Acked-by: Jason Gunthorpe <jgg@mellanox.com>
Jason
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-28 17:14 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <67c5f9b4-b24a-2806-e8d6-8b5241c66d6e@fb.com>
----- On Mar 28, 2018, at 12:43 PM, Alexei Starovoitov ast@fb.com wrote:
> On 3/28/18 6:49 AM, Mathieu Desnoyers wrote:
>> ----- On Mar 27, 2018, at 10:11 PM, Alexei Starovoitov ast@fb.com wrote:
>>
>>> From: Alexei Starovoitov <ast@kernel.org>
>>>
>>> compute number of arguments passed into tracepoint
>>> at compile time and store it as part of 'struct tracepoint'.
>>> The number is necessary to check safety of bpf program access that
>>> is coming in subsequent patch.
>>
>>
>> Hi Alexei,
>>
>> Given that only eBPF needs this parameter count, we can move
>> it to the struct bpf_raw_event_map newly introduced by Steven,
>> right ? This would reduce bloat of struct tracepoint. For instance,
>> we don't need to keep this count around when eBPF is configured
>> out.
>
> Makes sense. That is indeed cleaner. Will respin.
>
> What I don't like though is 'bloat' argument.
> 'u32 num_args' padded to 8-byte takes exactly the same amount
> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
> The number of these structures is the same as well
> and chances that tracepoints are on while bpf is off are slim.
> More so few emails ago you said:
> "I'm perfectly fine with adding the "num_args" stuff. I think it's
> really useful. It's only the for_each_kernel_tracepoint change for
> which I'm trying to understand the rationale."
>
> I'm guessing now you found out that num_args is not useful to lttng
> and it bloats its data structures?
I'm concerned about kernel configurations with only ftrace and
perf, with eBPF disabled. In those configurations, adding ebpf-specific
fields to struct tracepoint increases the kernel image size needlessly.
LTTng is not my concern in this discussion.
I'm also concerned about adding more and more elements to struct tracepoint
in general. For instance, I'd really like to see the regfunc/unregfunc
fields go away into a different section, allowing us to reduce the size of
struct tracepoint, so we can do a better use of the CPU caches when
tracing is enabled. The "funcs" field needs to be loaded each time a
tracepoint is fired, and therefore needs to be cache-hot. Adding more
cache-cold fields in there degrades cache locality, because "hot"
cache-lines then need to hold more cache-cold data.
> It's ok to change an opinion and I'm completely fine using that as
> a real reason.
Seeing the code of the new eBPF section provided by Steven made me realize
that we could use it to hold the num_args as well, keeping all the eBPF
data nice and tidy together. So yes, this is new information that changed
my opinion on the matter.
> Will repsin, as I said. No problem.
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 17:10 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328130407.7476cf17@gandalf.local.home>
On 3/28/18 10:04 AM, Steven Rostedt wrote:
> On Wed, 28 Mar 2018 09:43:56 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>>>
>>> Given that only eBPF needs this parameter count, we can move
>>> it to the struct bpf_raw_event_map newly introduced by Steven,
>>> right ? This would reduce bloat of struct tracepoint. For instance,
>>> we don't need to keep this count around when eBPF is configured
>>> out.
>>
>> Makes sense. That is indeed cleaner. Will respin.
>>
>> What I don't like though is 'bloat' argument.
>> 'u32 num_args' padded to 8-byte takes exactly the same amount
>> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
>> The number of these structures is the same as well
>> and chances that tracepoints are on while bpf is off are slim.
>> More so few emails ago you said:
>> "I'm perfectly fine with adding the "num_args" stuff. I think it's
>> really useful. It's only the for_each_kernel_tracepoint change for
>> which I'm trying to understand the rationale."
>
> I don't really care which one it goes in. The padding bloat is the same
> for both :-/ But I wonder if we can shrink it by doing a trick that
> Josh did in one of his patches. That is, to use a 32bit offset instead
> of a direct pointer. Since you are only accessing core kernel
> tracepoints.
>
> Thus, we could have
>
> struct bpf_raw_event_map {
> u32 tp_offset;
> u32 num_args;
> void *bpf_func;
> };
>
> and have:
>
> u64 tp_offset = (u64)tp - (u64)_sdata;
>
> if (WARN_ON(tp_offset > UINT_MAX)
> return -EINVAL;
>
> btp->tp_offset = (u32)tp_offset;
above math has to be build time constant, so warn_on likely
won't work.
imo the whole thing is too fragile and obscure.
I suggest to compress this 8 bytes * num_of_tracepoints later.
Especially would be good to do it in one way for
bpf_raw_event_map, ftrace and other places.
> And to get the tp, all you need to do is:
>
> tp = (struct tracepoint *)(btp->tp_offset + (unsigned long)_sdata);
>
> I've been thinking of doing this for other parts of the tracepoints and
> ftrace code.
>
> BTW, thanks for changing your code. I really appreciate it.
>
> -- Steve
>
^ permalink raw reply
* Re: [PATCH] sfp: allow cotsworks modules
From: Joe Perches @ 2018-03-28 17:05 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Andrew Lunn, Florian Fainelli, netdev
In-Reply-To: <20180328165154.GQ10980@n2100.armlinux.org.uk>
On Wed, 2018-03-28 at 17:51 +0100, Russell King - ARM Linux wrote:
> On Wed, Mar 28, 2018 at 09:19:01AM -0700, Joe Perches wrote:
> > On Wed, 2018-03-28 at 11:41 +0100, Russell King - ARM Linux wrote:
> > > On Wed, Mar 28, 2018 at 03:33:57AM -0700, Joe Perches wrote:
> > > > On Wed, 2018-03-28 at 11:18 +0100, Russell King wrote:
> > > > > Cotsworks modules fail the checksums - it appears that Cotsworks
> > > > > reprograms the EEPROM at the end of production with the final product
> > > > > information (serial, date code, and exact part number for module
> > > > > options) and fails to update the checksum.
> > > >
> > > > trivia:
> > > >
> > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> > > >
> > > > []
> > > > > @@ -574,23 +575,43 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
> > > >
> > > > []
> > > > > + if (cotsworks) {
> > > > > + dev_warn(sfp->dev,
> > > > > + "EEPROM base structure checksum failure (0x%02x != 0x%02x)\n",
> > > > > + check, id.base.cc_base);
> > > > > + } else {
> > > > > + dev_err(sfp->dev,
> > > > > + "EEPROM base structure checksum failure: 0x%02x != 0x%02x\n",
> > > >
> > > > It'd be better to move this above the if and
> > > > use only a single format string instead of
> > > > using 2 slightly different formats.
> > >
> > > No. I think you've missed the fact that one is a _warning_ the other is
> > > an _error_ and they are emitted at the appropriate severity. It's not
> > > just that the format strings are slightly different.
> >
> > Right. Still nicer to use the same formats.
>
> I'll stick a "Warning:" and "Error:" tag before them if you really
> want the rest of the message to be identically formatted - otherwise,
> when seeing reports from people's dmesg, there will be nothing to
> indicate which message was printed.
Not necessary. It was just a trivial size saving
from the format deduplication.
There is another dmesg info line
dev_info(sfp->dev, "module %.*s %.*s rev %.*s sn %.*s dc %.*s\n",
(int)sizeof(id.base.vendor_name), id.base.vendor_name,
(int)sizeof(id.base.vendor_pn), id.base.vendor_pn,
(int)sizeof(id.base.vendor_rev), id.base.vendor_rev,
(int)sizeof(id.ext.vendor_sn), id.ext.vendor_sn,
(int)sizeof(id.ext.datecode), id.ext.datecode);
the next line later which shows that it's a "COTSWORKS ".
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 17:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <67c5f9b4-b24a-2806-e8d6-8b5241c66d6e@fb.com>
On Wed, 28 Mar 2018 09:43:56 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> >
> > Given that only eBPF needs this parameter count, we can move
> > it to the struct bpf_raw_event_map newly introduced by Steven,
> > right ? This would reduce bloat of struct tracepoint. For instance,
> > we don't need to keep this count around when eBPF is configured
> > out.
>
> Makes sense. That is indeed cleaner. Will respin.
>
> What I don't like though is 'bloat' argument.
> 'u32 num_args' padded to 8-byte takes exactly the same amount
> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
> The number of these structures is the same as well
> and chances that tracepoints are on while bpf is off are slim.
> More so few emails ago you said:
> "I'm perfectly fine with adding the "num_args" stuff. I think it's
> really useful. It's only the for_each_kernel_tracepoint change for
> which I'm trying to understand the rationale."
I don't really care which one it goes in. The padding bloat is the same
for both :-/ But I wonder if we can shrink it by doing a trick that
Josh did in one of his patches. That is, to use a 32bit offset instead
of a direct pointer. Since you are only accessing core kernel
tracepoints.
Thus, we could have
struct bpf_raw_event_map {
u32 tp_offset;
u32 num_args;
void *bpf_func;
};
and have:
u64 tp_offset = (u64)tp - (u64)_sdata;
if (WARN_ON(tp_offset > UINT_MAX)
return -EINVAL;
btp->tp_offset = (u32)tp_offset;
And to get the tp, all you need to do is:
tp = (struct tracepoint *)(btp->tp_offset + (unsigned long)_sdata);
I've been thinking of doing this for other parts of the tracepoints and
ftrace code.
BTW, thanks for changing your code. I really appreciate it.
-- Steve
^ 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