* [PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
From: Heiner Kallweit @ 2018-11-08 21:54 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
using interrupts isn't possible if a driver defines neither
config_intr nor ack_interrupts callback. So we can replace checking
flag PHY_HAS_INTERRUPT with checking for these callbacks.
This allows to remove this flag from a lot of driver configs, let's
start with the Realtek driver.
Heiner Kallweit (2):
net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
net: phy: realtek: remove flag PHY_HAS_INTERRUPT from driver configs
drivers/net/phy/phy_device.c | 5 +++--
drivers/net/phy/realtek.c | 7 -------
2 files changed, 3 insertions(+), 9 deletions(-)
--
2.19.1
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-08 21:54 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108212940.wvfnkanxqs2owcy6@mini-arch>
On Thu, 8 Nov 2018 13:29:40 -0800, Stanislav Fomichev wrote:
> On 11/08, Jakub Kicinski wrote:
> > On Wed, 7 Nov 2018 21:39:57 -0800, Stanislav Fomichev wrote:
> > > This commit adds support for loading/attaching/detaching flow
> > > dissector program. The structure of the flow dissector program is
> > > assumed to be the same as in the selftests:
> > >
> > > * flow_dissector section with the main entry point
> > > * a bunch of tail call progs
> > > * a jmp_table map that is populated with the tail call progs
> >
> > Could you split the loadall changes and the flow_dissector changes into
> > two separate patches?
> Sure, will do, but let's first agree on the semantical differences of
> load vs loadall.
>
> So far *load* actually loads _all_ progs (via bpf_object__load), but pins
> only the first program. Is that what we want? I wonder whether the
> assumption there was that there is only single program in the object.
> Should we load only the first program in *load*?
>
> If we add *loadall*, then the difference would be:
> *load*:
> * loads all maps and only the first program, pins only the first
> program
> *loadall*:
> * loads all maps and all programs, pins everything (maps and programs)
>
> Is this the expected behavior?
Loading all programs and maps for "load" is just a libbpf limitation we
can remove at some point.
^ permalink raw reply
* Re: [net-next 06/12] i40e/ixgbe/igb: fail on new WoL flag setting WAKE_MAGICSECURE
From: Jeff Kirsher @ 2018-11-08 21:53 UTC (permalink / raw)
To: Michal Kubecek, Kevin Easton
Cc: davem, Todd Fujinaka, netdev, nhorman, sassmann
In-Reply-To: <20181108064250.GB24800@unicorn.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 4488 bytes --]
On Thu, 2018-11-08 at 07:42 +0100, Michal Kubecek wrote:
> On Thu, Nov 08, 2018 at 06:05:26AM +0000, Kevin Easton wrote:
> > On Wed, Nov 07, 2018 at 02:48:24PM -0800, Jeff Kirsher wrote:
> > > From: Todd Fujinaka <todd.fujinaka@intel.com>
> > >
> > > There's a new flag for setting WoL filters that is only
> > > enabled on one manufacturer's NICs, and it's not ours. Fail
> > > with EOPNOTSUPP.
> > >
> > > Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
> > > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > ---
> > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 3 ++-
> > > drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
> > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 3 ++-
> > > 3 files changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > index 9f8464f80783..9c1211ad2c6b 100644
> > > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > @@ -2377,7 +2377,8 @@ static int i40e_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > return -EOPNOTSUPP;
> > >
> > > /* only magic packet is supported */
> > > - if (wol->wolopts && (wol->wolopts != WAKE_MAGIC))
> > > + if (wol->wolopts && (wol->wolopts != WAKE_MAGIC)
> > > + | (wol->wolopts != WAKE_FILTER))
> > > return -EOPNOTSUPP;
> >
> > This doesn't look right. WAKE_MAGIC and WAKE_FILTER are distinct, so
> >
> > (wol->wolopts != WAKE_MAGIC) | (wol->wolopts != WAKE_FILTER)
> >
> > will always be 1.
>
> Right. Also, using "|" with logical values is rather confusing. While
> the result works as expected, its priority is higher than priority of
> && (which would not be true for ||), making the code counterintuitive.
>
> BtW, the patch subject is also wrong, the newly added flag it is dealing
> with is WAKE_FILTER, not WAKE_MAGICSECURE.
After looking into this this more, this does appear to be incorrect. The
author of this change is currently out on vacation, so once he gets back, I
will address both Kevin's and your concerns.
>
> > It looks like the existing test in this driver was fine - it *only*
> > accepted wol->wolopts of either 0 or WAKE_MAGIC, it was already
> > rejecting everything else including WAKE_FILTER.
>
> Another way to write the check would be
>
> if (wol->wolopts & ~WAKE_MAGIC)
>
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > index 5acf3b743876..c57671068245 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > @@ -2113,7 +2113,7 @@ static int igb_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > {
> > > struct igb_adapter *adapter = netdev_priv(netdev);
> > >
> > > - if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE))
> > > + if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE | WAKE_FILTER))
> > > return -EOPNOTSUPP;
> > >
> > > if (!(adapter->flags & IGB_FLAG_WOL_SUPPORTED))
>
> I would also suggest taking the opposite approach here, i.e. listing the
> flags which _are_ supported so that we don't have to update the code if
> another wol flag is added (or userspace sends an invalid one):
>
> #define SUPPORTED_WOL_MODES \
> (WAKE_PHY | WAKE_UCAST | WAKE_MCAST | WAKE_BCAST | WAKE_MAGIC)
> ...
> if (wol->wolopts & ~SUPPORTED_WOL_MODES)
> return -EOPNOTSUPP;
>
>
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > index 732b1e6ecc43..acba067cc15a 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > @@ -2206,7 +2206,8 @@ static int ixgbe_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > {
> > > struct ixgbe_adapter *adapter = netdev_priv(netdev);
> > >
> > > - if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE))
> > > + if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE |
> > > + WAKE_FILTER))
> > > return -EOPNOTSUPP;
> > >
> > > if (ixgbe_wol_exclusion(adapter, wol))
>
> ...and here.
>
> Michal Kubecek
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-08 21:52 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108212539.esqjw4k5hz2pxowu@mini-arch>
On Thu, 8 Nov 2018 13:25:39 -0800, Stanislav Fomichev wrote:
> > > > + goto err_close_obj;
> > > > + }
> > > > +
> > > > const char *sec_name = bpf_program__title(prog, false);
> > > >
> > > > err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
> > > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > > > goto err_close_obj;
> > > > }
> > > > }
> > > > - bpf_program__set_type(prog, attr.prog_type);
> > > > - bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > > > +
> > > > + bpf_object__for_each_program(pos, obj) {
> > > > + bpf_program__set_ifindex(pos, ifindex);
> > > > + bpf_program__set_type(pos, attr.prog_type);
> > > > + bpf_program__set_expected_attach_type(pos,
> > > > + expected_attach_type);
> > > > + }
> > >
> > > I still believe you can have programs of different types here, and be
> > > able to load them. I tried it and managed to have it working fine. If no
> > > type is provided from command line we can retrieve types for each
> > > program from its section name. If a type is provided on the command
> > > line, we can do the same, but I am not sure we should do it, or impose
> > > that type for all programs instead.
> >
> > attr->prog_type is one per object, though. How do we set that one?
> Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
> version in the bpf prog?
>
> It will probably work quite nicely for both of our options:
>
> * type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
> version (over cautious?)
> * type specified (and applied to all progs): using bpf_prog_type__needs_kver
> to require/not requqire kernel version
Right, but they you can't infer it from the program name, since there's
multiple.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-08 21:51 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108212012.wijumgzknr5b7gdx@mini-arch>
On Thu, 8 Nov 2018 13:20:12 -0800, Stanislav Fomichev wrote:
> On 11/08, Jakub Kicinski wrote:
> > On Thu, 8 Nov 2018 18:21:24 +0000, Quentin Monnet wrote:
> > > >>> @@ -79,8 +82,11 @@ DESCRIPTION
> > > >>> contain a dot character ('.'), which is reserved for future
> > > >>> extensions of *bpffs*.
> > > >>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > > >>> + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > > >>> Load bpf program from binary *OBJ* and pin as *FILE*.
> > > >>> + **bpftool prog load** will pin only the first bpf program
> > > >>> + from the *OBJ*, **bpftool prog loadall** will pin all maps
> > > >>> + and programs from the *OBJ*.
> > > >>
> > > >> This could be improved regarding maps: with "bpftool prog load" I think we
> > > >> also load and pin all maps, but your description implies this is only the
> > > >> case with "loadall"
> > > > I don't think we pin any maps with `bpftool prog load`, we certainly load
> > > > them, but we don't pin any afaict. Can you point me to the code where we
> > > > pin the maps?
> > > >
> > >
> > > My bad. I read "pin" but thought "load". It does not pin them indeed,
> > > sorry about that.
> >
> > Right, but I don't see much reason why prog loadall should pin maps.
> It does seem convenient to have an option to pin everything, so we
> don't require anything else to find the ids of the maps.
> If we are pinning all progs, might as well pin the maps, why not? See
> my example in the commit message, for example, where I just hard code
> the expected map name. Convenient :-)
Sure. I can see how its convenient to your use case. A lot of people
will be very used to finding out the maps because if program is loaded
with iproute2 tools neither programs nor maps get pinned.
Please add a "pinmaps MAP_DIR" optional parameter as a separate patch.
> > The reason to pin program(s) is to hold some reference and to be able
> > to find them. Since we have the programs pinned we should be able to
> > find their maps with relative ease.
> >
> > $ bpftool prog show pinned /sys/fs/bpf/prog
> > 7: cgroup_skb tag 2a142ef67aaad174 gpl
> > loaded_at 2018-11-08T11:02:25-0800 uid 0
> > xlated 296B jited 229B memlock 4096B map_ids 6,7
> >
> > possibly:
> >
> > $ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
> > 6
> >
> > Moreover, I think program and map names may collide making ELFs
> > unloadable..
> It doesn't sound like a big problem, sounds like a constraint we can live
> with.
^ permalink raw reply
* Re: [PATCH v3 1/2] kretprobe: produce sane stack traces
From: Masami Hiramatsu @ 2018-11-09 7:26 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Aleksa Sarai, Steven Rostedt, Naveen N. Rao, Anil S Keshavamurthy,
David S. Miller, Masami Hiramatsu, Jonathan Corbet,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, Brendan Gregg,
Christian Brauner, Aleksa Sarai, netd
In-Reply-To: <20181108144437.l3proaovfqm5osnr@treble>
On Thu, 8 Nov 2018 08:44:37 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Nov 08, 2018 at 07:04:48PM +1100, Aleksa Sarai wrote:
> > On 2018-11-08, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > I will attach what I have at the moment to hopefully explain what the
> > > issue I've found is (re-using the kretprobe architecture but with the
> > > shadow-stack idea).
> >
> > Here is the patch I have at the moment (it works, except for the
> > question I have about how to handle the top-level pt_regs -- I've marked
> > that code with XXX).
> >
> > --
> > Aleksa Sarai
> > Senior Software Engineer (Containers)
> > SUSE Linux GmbH
> > <https://www.cyphar.com/>
> >
> > --8<---------------------------------------------------------------------
> >
> > Since the return address is modified by kretprobe, the various unwinders
> > can produce invalid and confusing stack traces. ftrace mostly solved
> > this problem by teaching each unwinder how to find the original return
> > address for stack trace purposes. This same technique can be applied to
> > kretprobes by simply adding a pointer to where the return address was
> > replaced in the stack, and then looking up the relevant
> > kretprobe_instance when a stack trace is requested.
> >
> > [WIP: This is currently broken because the *first entry* will not be
> > overwritten since it looks like the stack pointer is different
> > when we are provided pt_regs. All other addresses are correctly
> > handled.]
>
> When you see this problem, what does regs->ip point to? If it's
> pointing to generated code, then we don't _currently_ have a way of
> dealing with that. If it's pointing to a real function, we can fix that
> with unwind hints.
As I replied, If the stackdump is called from kretprobe event, regs->ip
always points trampoline function. Otherwise (maybe from kprobe event,
or panic, BUG etc.) it always be the address which the event occurs.
So fixing regs->ip is correct.
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 1/2] kretprobe: produce sane stack traces
From: Masami Hiramatsu @ 2018-11-09 7:15 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Steven Rostedt, Naveen N. Rao, Anil S Keshavamurthy,
David S. Miller, Masami Hiramatsu, Jonathan Corbet,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, Brendan Gregg,
Christian Brauner, Aleksa Sarai, netdev, linux-doc
In-Reply-To: <20181108080448.rggfn4zawi3por23@yavin>
On Thu, 8 Nov 2018 19:04:48 +1100
Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-11-08, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > I will attach what I have at the moment to hopefully explain what the
> > issue I've found is (re-using the kretprobe architecture but with the
> > shadow-stack idea).
>
> Here is the patch I have at the moment (it works, except for the
> question I have about how to handle the top-level pt_regs -- I've marked
> that code with XXX).
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
>
> --8<---------------------------------------------------------------------
>
> Since the return address is modified by kretprobe, the various unwinders
> can produce invalid and confusing stack traces. ftrace mostly solved
> this problem by teaching each unwinder how to find the original return
> address for stack trace purposes. This same technique can be applied to
> kretprobes by simply adding a pointer to where the return address was
> replaced in the stack, and then looking up the relevant
> kretprobe_instance when a stack trace is requested.
>
> [WIP: This is currently broken because the *first entry* will not be
> overwritten since it looks like the stack pointer is different
> when we are provided pt_regs. All other addresses are correctly
> handled.]
>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> arch/x86/events/core.c | 6 +++-
> arch/x86/include/asm/ptrace.h | 1 +
> arch/x86/kernel/kprobes/core.c | 5 ++--
> arch/x86/kernel/stacktrace.c | 10 +++++--
> arch/x86/kernel/unwind_frame.c | 2 ++
> arch/x86/kernel/unwind_guess.c | 5 +++-
> arch/x86/kernel/unwind_orc.c | 2 ++
> include/linux/kprobes.h | 15 +++++++++-
> kernel/kprobes.c | 55 ++++++++++++++++++++++++++++++++++
> 9 files changed, 93 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index de32741d041a..d71062702179 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2371,7 +2371,11 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
> return;
> }
>
> - if (perf_callchain_store(entry, regs->ip))
> + /* XXX: Currently broken -- stack_addr(regs) doesn't match entry. */
> + addr = regs->ip;
> + //addr = ftrace_graph_ret_addr(current, &state.graph_idx, addr, stack_addr(regs));
> + addr = kretprobe_ret_addr(current, addr, stack_addr(regs));
> + if (perf_callchain_store(entry, addr))
> return;
>
> for (unwind_start(&state, current, regs, NULL); !unwind_done(&state);
> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> index ee696efec99f..c4dfafd43e11 100644
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -172,6 +172,7 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
> return regs->sp;
> }
> #endif
> +#define stack_addr(regs) ((unsigned long *) kernel_stack_pointer(regs))
No, you should use kernel_stack_pointer(regs) itself instead of stack_addr().
>
> #define GET_IP(regs) ((regs)->ip)
> #define GET_FP(regs) ((regs)->bp)
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index b0d1e81c96bb..eb4da885020c 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -69,8 +69,6 @@
> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>
> -#define stack_addr(regs) ((unsigned long *)kernel_stack_pointer(regs))
I don't like keeping this meaningless macro... this should be replaced with generic
kernel_stack_pointer() macro.
> -
> #define W(row, b0, b1, b2, b3, b4, b5, b6, b7, b8, b9, ba, bb, bc, bd, be, bf)\
> (((b0##UL << 0x0)|(b1##UL << 0x1)|(b2##UL << 0x2)|(b3##UL << 0x3) | \
> (b4##UL << 0x4)|(b5##UL << 0x5)|(b6##UL << 0x6)|(b7##UL << 0x7) | \
> @@ -568,7 +566,8 @@ void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs)
> {
> unsigned long *sara = stack_addr(regs);
>
> - ri->ret_addr = (kprobe_opcode_t *) *sara;
> + ri->ret_addrp = (kprobe_opcode_t **) sara;
> + ri->ret_addr = *ri->ret_addrp;
>
> /* Replace the return addr with trampoline addr */
> *sara = (unsigned long) &kretprobe_trampoline;
> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> index 7627455047c2..8a4fb3109d6b 100644
> --- a/arch/x86/kernel/stacktrace.c
> +++ b/arch/x86/kernel/stacktrace.c
> @@ -8,6 +8,7 @@
> #include <linux/sched/task_stack.h>
> #include <linux/stacktrace.h>
> #include <linux/export.h>
> +#include <linux/kprobes.h>
> #include <linux/uaccess.h>
> #include <asm/stacktrace.h>
> #include <asm/unwind.h>
> @@ -37,8 +38,13 @@ static void noinline __save_stack_trace(struct stack_trace *trace,
> struct unwind_state state;
> unsigned long addr;
>
> - if (regs)
> - save_stack_address(trace, regs->ip, nosched);
> + if (regs) {
> + /* XXX: Currently broken -- stack_addr(regs) doesn't match entry. */
> + addr = regs->ip;
Since this part is for storing regs->ip as a top of call-stack, this
seems correct code. Stack unwind will be done next block.
> + //addr = ftrace_graph_ret_addr(current, &state.graph_idx, addr, stack_addr(regs));
so func graph return trampoline address will be shown only when unwinding stack entries.
I mean func-graph tracer is not used as an event, so it never kicks stackdump.
> + addr = kretprobe_ret_addr(current, addr, stack_addr(regs));
But since kretprobe will be an event, which can kick the stackdump.
BTW, from kretprobe, regs->ip should always be the trampoline handler,
see arch/x86/kernel/kprobes/core.c:772 :-)
So it must be fixed always.
> + save_stack_address(trace, addr, nosched);
> + }
>
> for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
> unwind_next_frame(&state)) {
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index 3dc26f95d46e..47062427e9a3 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -1,4 +1,5 @@
> #include <linux/sched.h>
> +#include <linux/kprobes.h>
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/interrupt.h>
> @@ -270,6 +271,7 @@ static bool update_stack_state(struct unwind_state *state,
> addr = READ_ONCE_TASK_STACK(state->task, *addr_p);
> state->ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
> addr, addr_p);
> + state->ip = kretprobe_ret_addr(state->task, state->ip, addr_p);
> }
>
> /* Save the original stack pointer for unwind_dump(): */
> diff --git a/arch/x86/kernel/unwind_guess.c b/arch/x86/kernel/unwind_guess.c
> index 4f0e17b90463..554fd7c5c331 100644
> --- a/arch/x86/kernel/unwind_guess.c
> +++ b/arch/x86/kernel/unwind_guess.c
> @@ -1,5 +1,6 @@
> #include <linux/sched.h>
> #include <linux/ftrace.h>
> +#include <linux/kprobes.h>
> #include <asm/ptrace.h>
> #include <asm/bitops.h>
> #include <asm/stacktrace.h>
> @@ -14,8 +15,10 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
>
> addr = READ_ONCE_NOCHECK(*state->sp);
>
> - return ftrace_graph_ret_addr(state->task, &state->graph_idx,
> + addr = ftrace_graph_ret_addr(state->task, &state->graph_idx,
> addr, state->sp);
> + addr = kretprobe_ret_addr(state->task, addr, state->sp);
> + return addr;
> }
> EXPORT_SYMBOL_GPL(unwind_get_return_address);
>
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 26038eacf74a..b6393500d505 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -1,5 +1,6 @@
> #include <linux/module.h>
> #include <linux/sort.h>
> +#include <linux/kprobes.h>
> #include <asm/ptrace.h>
> #include <asm/stacktrace.h>
> #include <asm/unwind.h>
> @@ -462,6 +463,7 @@ bool unwind_next_frame(struct unwind_state *state)
>
> state->ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
> state->ip, (void *)ip_p);
> + state->ip = kretprobe_ret_addr(state->task, state->ip, (void *)ip_p);
>
> state->sp = sp;
> state->regs = NULL;
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index e909413e4e38..3a01f9998064 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -172,6 +172,7 @@ struct kretprobe_instance {
> struct hlist_node hlist;
> struct kretprobe *rp;
> kprobe_opcode_t *ret_addr;
> + kprobe_opcode_t **ret_addrp;
> struct task_struct *task;
> char data[0];
> };
> @@ -203,6 +204,7 @@ static inline int kprobes_built_in(void)
> extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
> struct pt_regs *regs);
> extern int arch_trampoline_kprobe(struct kprobe *p);
> +extern void kretprobe_trampoline(void);
> #else /* CONFIG_KRETPROBES */
> static inline void arch_prepare_kretprobe(struct kretprobe *rp,
> struct pt_regs *regs)
> @@ -212,6 +214,9 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
> {
> return 0;
> }
> +static inline void kretprobe_trampoline(void)
> +{
> +}
> #endif /* CONFIG_KRETPROBES */
>
> extern struct kretprobe_blackpoint kretprobe_blacklist[];
> @@ -341,7 +346,7 @@ struct kprobe *get_kprobe(void *addr);
> void kretprobe_hash_lock(struct task_struct *tsk,
> struct hlist_head **head, unsigned long *flags);
> void kretprobe_hash_unlock(struct task_struct *tsk, unsigned long *flags);
> -struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);
> +struct hlist_head *kretprobe_inst_table_head(struct task_struct *tsk);
>
> /* kprobe_running() will just return the current_kprobe on this CPU */
> static inline struct kprobe *kprobe_running(void)
> @@ -371,6 +376,9 @@ void unregister_kretprobe(struct kretprobe *rp);
> int register_kretprobes(struct kretprobe **rps, int num);
> void unregister_kretprobes(struct kretprobe **rps, int num);
>
> +unsigned long kretprobe_ret_addr(struct task_struct *tsk, unsigned long ret,
> + unsigned long *retp);
> +
> void kprobe_flush_task(struct task_struct *tk);
> void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>
> @@ -425,6 +433,11 @@ static inline void unregister_kretprobe(struct kretprobe *rp)
> static inline void unregister_kretprobes(struct kretprobe **rps, int num)
> {
> }
> +unsigned long kretprobe_ret_addr(struct task_struct *task, unsigned long ret,
> + unsigned long *retp)
> +{
> + return ret;
> +}
> static inline void kprobe_flush_task(struct task_struct *tk)
> {
> }
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 90e98e233647..ed78141664ec 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -83,6 +83,11 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
> return &(kretprobe_table_locks[hash].lock);
> }
>
> +struct hlist_head *kretprobe_inst_table_head(struct task_struct *tsk)
> +{
> + return &kretprobe_inst_table[hash_ptr(tsk, KPROBE_HASH_BITS)];
> +}
> +
> /* Blacklist -- list of struct kprobe_blacklist_entry */
> static LIST_HEAD(kprobe_blacklist);
>
> @@ -1206,6 +1211,15 @@ __releases(hlist_lock)
> }
> NOKPROBE_SYMBOL(kretprobe_table_unlock);
>
> +static bool kretprobe_hash_is_locked(struct task_struct *tsk)
> +{
> + unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> + raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
> +
> + return raw_spin_is_locked(hlist_lock);
> +}
> +NOKPROBE_SYMBOL(kretprobe_hash_is_locked);
> +
> /*
> * This function is called from finish_task_switch when task tk becomes dead,
> * so that we can recycle any function-return probe instances associated
> @@ -1856,6 +1870,41 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +unsigned long kretprobe_ret_addr(struct task_struct *tsk, unsigned long ret,
> + unsigned long *retp)
> +{
> + struct kretprobe_instance *ri;
> + unsigned long flags = 0;
> + struct hlist_head *head;
> + bool need_lock;
> +
> + if (likely(ret != (unsigned long) &kretprobe_trampoline))
> + return ret;
> +
> + need_lock = !kretprobe_hash_is_locked(tsk);
> + if (WARN_ON(need_lock))
> + kretprobe_hash_lock(tsk, &head, &flags);
> + else
> + head = kretprobe_inst_table_head(tsk);
This may not work unless this is called from the kretprobe handler context,
since if we are out of kretprobe handler context, another CPU can lock the
hash table and it can be detected by kretprobe_hash_is_locked();.
So, we should check we are in the kretprobe handler context if tsk == current,
if not, we definately can lock the hash lock without any warning. This can
be something like;
if (is_kretprobe_handler_context()) {
// kretprobe_hash_lock(current == tsk) has been locked by caller
if (tsk != current && kretprobe_hash(tsk) != kretprobe_hash(current))
// the hash of tsk and current can be same.
need_lock = true;
} else
// we should take a lock for tsk.
need_lock = true;
Thank you,
> +
> + hlist_for_each_entry(ri, head, hlist) {
> + if (ri->task != current)
> + continue;
> + if (ri->ret_addr == (kprobe_opcode_t *) &kretprobe_trampoline)
> + continue;
> + if (ri->ret_addrp == (kprobe_opcode_t **) retp) {
> + ret = (unsigned long) ri->ret_addr;
> + goto out;
> + }
> + }
> +
> +out:
> + if (need_lock)
> + kretprobe_hash_unlock(tsk, &flags);
> + return ret;
> +}
> +NOKPROBE_SYMBOL(kretprobe_ret_addr);
> +
> bool __weak arch_kprobe_on_func_entry(unsigned long offset)
> {
> return !offset;
> @@ -2005,6 +2054,12 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +unsigned long kretprobe_ret_addr(struct task_struct *tsk, unsigned long ret,
> + unsigned long *retp)
> +{
> + return ret;
> +}
> +NOKPROBE_SYMBOL(kretprobe_ret_addr);
> #endif /* CONFIG_KRETPROBES */
>
> /* Set the kprobe gone and remove its instruction buffer. */
> --
> 2.19.1
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* [PATCH net-next] net: phy: improve struct phy_device member interrupts handling
From: Heiner Kallweit @ 2018-11-08 21:36 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
As a heritage from the very early days of phylib member interrupts is
defined as u32 even though it's just a flag whether interrupts are
enabled. So we can change it to a bitfield member. In addition change
the code dealing with this member in a way that it's clear we're
dealing with a bool value.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
Actually this member isn't needed at all and could be replaced with
a parameter in phy_driver->config_intr. But this would mean an API
change, maybe I come up with a proposal later.
---
drivers/net/phy/phy.c | 2 +-
include/linux/phy.h | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index dd5bff955..a5e6acfe6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -115,7 +115,7 @@ static int phy_clear_interrupt(struct phy_device *phydev)
*
* Returns 0 on success or < 0 on error.
*/
-static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
+static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
{
phydev->interrupts = interrupts;
if (phydev->drv->config_intr)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5dd85c441..fc90af152 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -263,8 +263,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
void devm_mdiobus_free(struct device *dev, struct mii_bus *bus);
struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
-#define PHY_INTERRUPT_DISABLED 0x0
-#define PHY_INTERRUPT_ENABLED 0x80000000
+#define PHY_INTERRUPT_DISABLED 0
+#define PHY_INTERRUPT_ENABLED 1
/* PHY state machine states:
*
@@ -410,6 +410,9 @@ struct phy_device {
/* The most recently read link state */
unsigned link:1;
+ /* Interrupts are enabled */
+ unsigned interrupts:1;
+
enum phy_state state;
u32 dev_flags;
@@ -425,9 +428,6 @@ struct phy_device {
int pause;
int asym_pause;
- /* Enabled Interrupts */
- u32 interrupts;
-
/* Union of PHY and Attached devices' supported modes */
/* See mii.h for more info */
u32 supported;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-08 21:29 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108114630.28907570@cakuba.hsd1.ca.comcast.net>
On 11/08, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 21:39:57 -0800, Stanislav Fomichev wrote:
> > This commit adds support for loading/attaching/detaching flow
> > dissector program. The structure of the flow dissector program is
> > assumed to be the same as in the selftests:
> >
> > * flow_dissector section with the main entry point
> > * a bunch of tail call progs
> > * a jmp_table map that is populated with the tail call progs
>
> Could you split the loadall changes and the flow_dissector changes into
> two separate patches?
Sure, will do, but let's first agree on the semantical differences of
load vs loadall.
So far *load* actually loads _all_ progs (via bpf_object__load), but pins
only the first program. Is that what we want? I wonder whether the
assumption there was that there is only single program in the object.
Should we load only the first program in *load*?
If we add *loadall*, then the difference would be:
*load*:
* loads all maps and only the first program, pins only the first
program
*loadall*:
* loads all maps and all programs, pins everything (maps and programs)
Is this the expected behavior?
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-08 21:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108114544.1ac0cd44@cakuba.hsd1.ca.comcast.net>
On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 11:16:43 +0000, Quentin Monnet wrote:
> > > - bpf_program__set_ifindex(prog, ifindex);
> > > if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > > + if (!prog) {
> > > + p_err("can not guess program type when loading all programs\n");
>
> No new lines in p_err(), beaks JSON.
Thanks, I'll probably remove that altogether and do auto inference of
prog_type from the section name here.
> > > + goto err_close_obj;
> > > + }
> > > +
> > > const char *sec_name = bpf_program__title(prog, false);
> > >
> > > err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
> > > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > > goto err_close_obj;
> > > }
> > > }
> > > - bpf_program__set_type(prog, attr.prog_type);
> > > - bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > > +
> > > + bpf_object__for_each_program(pos, obj) {
> > > + bpf_program__set_ifindex(pos, ifindex);
> > > + bpf_program__set_type(pos, attr.prog_type);
> > > + bpf_program__set_expected_attach_type(pos,
> > > + expected_attach_type);
> > > + }
> >
> > I still believe you can have programs of different types here, and be
> > able to load them. I tried it and managed to have it working fine. If no
> > type is provided from command line we can retrieve types for each
> > program from its section name. If a type is provided on the command
> > line, we can do the same, but I am not sure we should do it, or impose
> > that type for all programs instead.
>
> attr->prog_type is one per object, though. How do we set that one?
Isn't it used only in __bpf_object__open_xattr to require/not-require kernel
version in the bpf prog?
It will probably work quite nicely for both of our options:
* type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel
version (over cautious?)
* type specified (and applied to all progs): using bpf_prog_type__needs_kver
to require/not requqire kernel version
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Improve handling of ENOENT on map dumps
From: Jakub Kicinski @ 2018-11-08 21:25 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, David Ahern
In-Reply-To: <20181108210007.6576-1-dsahern@kernel.org>
On Thu, 8 Nov 2018 13:00:07 -0800, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> bpftool output is not user friendly when dumping a map with only a few
> populated entries:
>
> $ bpftool map
> 1: devmap name tx_devmap flags 0x0
> key 4B value 4B max_entries 64 memlock 4096B
> 2: array name tx_idxmap flags 0x0
> key 4B value 4B max_entries 64 memlock 4096B
>
> $ bpftool map dump id 1
> key:
> 00 00 00 00
> value:
> No such file or directory
> key:
> 01 00 00 00
> value:
> No such file or directory
> key:
> 02 00 00 00
> value:
> No such file or directory
> key: 03 00 00 00 value: 03 00 00 00
>
> Handle ENOENT by keeping the line format sane and dumping
> "<no entry>" for the value
>
> $ bpftool map dump id 1
> key: 00 00 00 00 value: <no entry>
> key: 01 00 00 00 value: <no entry>
> key: 02 00 00 00 value: <no entry>
> key: 03 00 00 00 value: 03 00 00 00
> ...
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Seems good. I wonder why "fd" maps report all indexes in get_next..
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Alternatively, could just omit the value, so:
> key: 00 00 00 00 value:
> key: 01 00 00 00 value:
> key: 02 00 00 00 value:
> key: 03 00 00 00 value: 03 00 00 00
>
> tools/bpf/bpftool/map.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 101b8a881225..1f0060644e0c 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -383,7 +383,10 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
> printf(single_line ? " " : "\n");
>
> printf("value:%c", break_names ? '\n' : ' ');
> - fprint_hex(stdout, value, info->value_size, " ");
> + if (value)
> + fprint_hex(stdout, value, info->value_size, " ");
> + else
> + printf("<no entry>");
>
> printf("\n");
> } else {
> @@ -398,8 +401,12 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
> for (i = 0; i < n; i++) {
> printf("value (CPU %02d):%c",
> i, info->value_size > 16 ? '\n' : ' ');
> - fprint_hex(stdout, value + i * step,
> - info->value_size, " ");
> + if (value) {
> + fprint_hex(stdout, value + i * step,
> + info->value_size, " ");
> + } else {
> + printf("<no entry>");
> + }
nit: in other places you don't add { }
> printf("\n");
> }
> }
> @@ -731,7 +738,11 @@ static int dump_map_elem(int fd, void *key, void *value,
> jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
> jsonw_end_object(json_wtr);
> } else {
> - print_entry_error(map_info, key, strerror(lookup_errno));
> + if (errno == ENOENT)
> + print_entry_plain(map_info, key, NULL);
> + else
> + print_entry_error(map_info, key,
> + strerror(lookup_errno));
> }
>
> return 0;
^ permalink raw reply
* Re: [PATCH net-next 0/7] net: sched: prepare for more Qdisc offloads
From: Toke Høiland-Jørgensen @ 2018-11-08 21:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, oss-drivers, jiri, xiyou.wangcong, jhs,
nogah.frankel
In-Reply-To: <20181108080254.1a2af2ae@cakuba.hsd1.ca.comcast.net>
Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> On Thu, 08 Nov 2018 12:48:27 +0100, Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <jakub.kicinski@netronome.com> writes:
>> > Hi!
>> >
>> > This series refactors the "switchdev" Qdisc offloads a little. We have
>> > a few Qdiscs which can be fully offloaded today to the forwarding plane
>> > of switching devices.
>> >
>> > First patch adds a helper for handing statistic dumps, the code seems
>> > to be copy pasted between PRIO and RED.
>>
>> Hi Jakub
>>
>> I didn't know there was offload capabilities for AQMs, that's cool! Do
>> you have any plans to add offloads for any of the modern AQMs (CoDel or
>> PIE)?
>
> I'd really like to add CoDel offload, but it's not a plan at this
> point :(
Right, too bad. Well, here's hoping that you'll get the chance in the
not too distant future :)
-Toke
^ permalink raw reply
* Re: [PATCH] xen/netfront: remove unnecessary wmb
From: Juergen Gross @ 2018-11-09 6:58 UTC (permalink / raw)
To: Jacob Wen, netdev; +Cc: xen-devel, linux-kernel
In-Reply-To: <20181109065359.14900-1-jian.w.wen@oracle.com>
On 09/11/2018 07:53, Jacob Wen wrote:
> RING_PUSH_REQUESTS_AND_CHECK_NOTIFY is already able to make sure backend sees
> requests before req_prod is updated.
>
> Signed-off-by: Jacob Wen <jian.w.wen@oracle.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-08 21:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108113503.0f4ce2e7@cakuba.hsd1.ca.comcast.net>
On 11/08, Jakub Kicinski wrote:
> On Thu, 8 Nov 2018 18:21:24 +0000, Quentin Monnet wrote:
> > >>> @@ -79,8 +82,11 @@ DESCRIPTION
> > >>> contain a dot character ('.'), which is reserved for future
> > >>> extensions of *bpffs*.
> > >>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > >>> + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > >>> Load bpf program from binary *OBJ* and pin as *FILE*.
> > >>> + **bpftool prog load** will pin only the first bpf program
> > >>> + from the *OBJ*, **bpftool prog loadall** will pin all maps
> > >>> + and programs from the *OBJ*.
> > >>
> > >> This could be improved regarding maps: with "bpftool prog load" I think we
> > >> also load and pin all maps, but your description implies this is only the
> > >> case with "loadall"
> > > I don't think we pin any maps with `bpftool prog load`, we certainly load
> > > them, but we don't pin any afaict. Can you point me to the code where we
> > > pin the maps?
> > >
> >
> > My bad. I read "pin" but thought "load". It does not pin them indeed,
> > sorry about that.
>
> Right, but I don't see much reason why prog loadall should pin maps.
It does seem convenient to have an option to pin everything, so we
don't require anything else to find the ids of the maps.
If we are pinning all progs, might as well pin the maps, why not? See
my example in the commit message, for example, where I just hard code
the expected map name. Convenient :-)
> The reason to pin program(s) is to hold some reference and to be able
> to find them. Since we have the programs pinned we should be able to
> find their maps with relative ease.
>
> $ bpftool prog show pinned /sys/fs/bpf/prog
> 7: cgroup_skb tag 2a142ef67aaad174 gpl
> loaded_at 2018-11-08T11:02:25-0800 uid 0
> xlated 296B jited 229B memlock 4096B map_ids 6,7
>
> possibly:
>
> $ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
> 6
>
> Moreover, I think program and map names may collide making ELFs
> unloadable..
It doesn't sound like a big problem, sounds like a constraint we can live
with.
^ permalink raw reply
* [PATCH] xen/netfront: remove unnecessary wmb
From: Jacob Wen @ 2018-11-09 6:53 UTC (permalink / raw)
To: netdev; +Cc: jgross, xen-devel, linux-kernel
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY is already able to make sure backend sees
requests before req_prod is updated.
Signed-off-by: Jacob Wen <jian.w.wen@oracle.com>
---
drivers/net/xen-netfront.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index f17f602e6171..a8303afa15f1 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -337,8 +337,6 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
return;
}
- wmb(); /* barrier so backend seens requests */
-
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&queue->rx, notify);
if (notify)
notify_remote_via_irq(queue->rx_irq);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 00/20] octeontx2-af: NPC MCAM support and FLR handling
From: Arnd Bergmann @ 2018-11-08 21:02 UTC (permalink / raw)
To: Sunil Kovvuri; +Cc: Networking, David Miller, linux-soc, Sunil Goutham
In-Reply-To: <1541702161-30673-1-git-send-email-sunil.kovvuri@gmail.com>
On Thu, Nov 8, 2018 at 7:36 PM <sunil.kovvuri@gmail.com> wrote:
>
> From: Sunil Goutham <sgoutham@marvell.com>
Hmm, I noticed that you use a different address as the patch author
and the submitter. I'm guessing that "Sunil Goutham" and
"Sunil Kovvuri" actually refer to the same person, and you just
need to pick which of the two email addresses you want to use
for public communication, but that's not obvious here.
However, if there are actually two different Sunil's here, then
you need to add that second Signed-off-by.
I've taken a look at all the patches now, and found very little
sticking out that warranted a comment from my side, and
no real show-stoppers. That said, I found this series overall
much harder to understand than the previous ones, and don't
even know what to ask about it. My feeling is that it's probably
all fine, but that is purely based on a review of the individual
pieces, not the overall design and how they fit together. With the
earlier patches that I managed to get a better understanding
of, that seemed reasonable as well.
Arnd
^ permalink raw reply
* [PATCH bpf-next] bpftool: Improve handling of ENOENT on map dumps
From: David Ahern @ 2018-11-08 21:00 UTC (permalink / raw)
To: jakub.kicinski, netdev; +Cc: David Ahern
From: David Ahern <dsahern@gmail.com>
bpftool output is not user friendly when dumping a map with only a few
populated entries:
$ bpftool map
1: devmap name tx_devmap flags 0x0
key 4B value 4B max_entries 64 memlock 4096B
2: array name tx_idxmap flags 0x0
key 4B value 4B max_entries 64 memlock 4096B
$ bpftool map dump id 1
key:
00 00 00 00
value:
No such file or directory
key:
01 00 00 00
value:
No such file or directory
key:
02 00 00 00
value:
No such file or directory
key: 03 00 00 00 value: 03 00 00 00
Handle ENOENT by keeping the line format sane and dumping
"<no entry>" for the value
$ bpftool map dump id 1
key: 00 00 00 00 value: <no entry>
key: 01 00 00 00 value: <no entry>
key: 02 00 00 00 value: <no entry>
key: 03 00 00 00 value: 03 00 00 00
...
Signed-off-by: David Ahern <dsahern@gmail.com>
---
Alternatively, could just omit the value, so:
key: 00 00 00 00 value:
key: 01 00 00 00 value:
key: 02 00 00 00 value:
key: 03 00 00 00 value: 03 00 00 00
tools/bpf/bpftool/map.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 101b8a881225..1f0060644e0c 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -383,7 +383,10 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
printf(single_line ? " " : "\n");
printf("value:%c", break_names ? '\n' : ' ');
- fprint_hex(stdout, value, info->value_size, " ");
+ if (value)
+ fprint_hex(stdout, value, info->value_size, " ");
+ else
+ printf("<no entry>");
printf("\n");
} else {
@@ -398,8 +401,12 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
for (i = 0; i < n; i++) {
printf("value (CPU %02d):%c",
i, info->value_size > 16 ? '\n' : ' ');
- fprint_hex(stdout, value + i * step,
- info->value_size, " ");
+ if (value) {
+ fprint_hex(stdout, value + i * step,
+ info->value_size, " ");
+ } else {
+ printf("<no entry>");
+ }
printf("\n");
}
}
@@ -731,7 +738,11 @@ static int dump_map_elem(int fd, void *key, void *value,
jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
jsonw_end_object(json_wtr);
} else {
- print_entry_error(map_info, key, strerror(lookup_errno));
+ if (errno == ENOENT)
+ print_entry_plain(map_info, key, NULL);
+ else
+ print_entry_error(map_info, key,
+ strerror(lookup_errno));
}
return 0;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match
From: Andrew Lunn @ 2018-11-08 20:53 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <072cad0e-f5c0-2f40-b18d-8801834e2676@gmail.com>
> > Maybe we can find a clever way with a macro to specify only the PHY OUI
> > and compute a suitable mask automatically?
> >
> I don't think so. For Realtek each driver is specific even to a model
> revision (therefore mask 0xffffffff). Same applies to intel-xway.
> In broadcom.c we have masks 0xfffffff0, so for each model, independent
> of revision number. There is no general rule.
> Also we can't simply check for the first-bit-set to derive a mask.
I'm crystal ball gazing, but i think Florian was meaning something like
#define PHY_ID_UNIQUE(_id) \
.phy_id = _id_; \
.phy_id_mask = 0xffffffff;
It is the boilerplate setting .phy_id_mask which you don't like. This removes that boilerplate.
Andrew
^ permalink raw reply
* Re: [PATCH bpf-next v4 02/13] bpf: btf: Add BTF_KIND_FUNC
From: Edward Cree @ 2018-11-08 20:52 UTC (permalink / raw)
To: Yonghong Song, ast, daniel, netdev; +Cc: kernel-team, Martin KaFai Lau
In-Reply-To: <20181108203657.3138259-3-yhs@fb.com>
On 08/11/18 20:36, Yonghong Song wrote:
> This patch adds BTF_KIND_FUNC support to the type section.
> BTF_KIND_FUNC is used to specify the signature of a
> defined subprogram or the pointee of a function pointer.
>
> In BTF, the function type related data structures are
> struct bpf_param {
> __u32 name_off; /* parameter name */
> __u32 type; /* parameter type */
> };
> struct bpf_type {
> __u32 name_off; /* function name */
> __u32 info; /* BTF_KIND_FUNC and num of parameters (#vlen) */
> __u32 type; /* return type */
> }
> The data layout of the function type:
> struct bpf_type
> #vlen number of bpf_param's
>
> For a defined subprogram with valid function body,
> . function name and all parameter names except the vararg
> must be valid C identifier.
Given that there's an intention to support other frontends besides
C, what's the reason for this restriction?
> For the pointee of a function pointer,
> . function name and all parameter names will
> have name_off = 0 to indicate a non-existing name.
Why can't function pointer parameters have names?
E.g. imagine something like struct net_device_ops. All those
function pointers have named parameters and that's relevant info
when debugging.
-Ed
^ permalink raw reply
* Re: [PATCH 13/20] octeontx2-af: Add FLR interrupt handler
From: Arnd Bergmann @ 2018-11-08 20:50 UTC (permalink / raw)
To: Sunil Kovvuri
Cc: Networking, David Miller, linux-soc, Geetha sowjanya,
Sunil Goutham
In-Reply-To: <1541702161-30673-14-git-send-email-sunil.kovvuri@gmail.com>
On Thu, Nov 8, 2018 at 7:37 PM <sunil.kovvuri@gmail.com> wrote:
>
> From: Geetha sowjanya <gakula@marvell.com>
>
> All RVU PF's upon receiving a FLR will trigger an IRQ to RVU AF.
> This patch enables all RVU PF's FLR interrupt and registers a
> handler. Upon receiving an IRQ a workqueue is scheduled to
> cleanup all RVU blocks being used by the PF/VF which received
> the FLR.
I assume this all makes sense, but I find it hard to read the changelog
text with all those acronyms. I probably knew what an FLR and a RVU
is in the earlier review rounds, but it would be nice to reword changelogs
like this one so they make more sense to readers that are not already
familiar with the rest of the driver.
Arnd
^ permalink raw reply
* Re: [PATCH 11/20] octeontx2-af: Add support for stripping STAG/CTAG
From: Arnd Bergmann @ 2018-11-08 20:47 UTC (permalink / raw)
To: Sunil Kovvuri
Cc: Networking, David Miller, linux-soc, tduszynski, Sunil Goutham
In-Reply-To: <1541702161-30673-12-git-send-email-sunil.kovvuri@gmail.com>
On Thu, Nov 8, 2018 at 7:37 PM <sunil.kovvuri@gmail.com> wrote:
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> index f98b011..3f7e5e6 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> @@ -259,4 +259,34 @@ struct nix_rx_action {
> #endif
> };
>
> +struct nix_rx_vtag_action {
> +#if defined(__BIG_ENDIAN_BITFIELD)
> + u64 rsvd_63_48 :16;
> + u64 vtag1_valid :1;
> + u64 vtag1_type :3;
> + u64 rsvd_43 :1;
> + u64 vtag1_lid :3;
> + u64 vtag1_relptr :8;
> + u64 rsvd_31_16 :16;
> + u64 vtag0_valid :1;
> + u64 vtag0_type :3;
> + u64 rsvd_11 :1;
> + u64 vtag0_lid :3;
> + u64 vtag0_relptr :8;
> +#else
> + u64 vtag0_relptr :8;
> + u64 vtag0_lid :3;
> + u64 rsvd_11 :1;
> + u64 vtag0_type :3;
> + u64 vtag0_valid :1;
> + u64 rsvd_31_16 :16;
> + u64 vtag1_relptr :8;
> + u64 vtag1_lid :3;
> + u64 rsvd_43 :1;
> + u64 vtag1_type :3;
> + u64 vtag1_valid :1;
> + u64 rsvd_63_48 :16;
> +#endif
> +};
Here is another instance of bitfields in an interface structure. As
before, please try to avoid doing that and use bit shifts and masks
instead.
Arnd
^ permalink raw reply
* Re: [PATCH 08/20] octeontx2-af: Alloc and config NPC MCAM entry at a time
From: Arnd Bergmann @ 2018-11-08 20:43 UTC (permalink / raw)
To: Sunil Kovvuri; +Cc: Networking, David Miller, linux-soc, Sunil Goutham
In-Reply-To: <1541702161-30673-9-git-send-email-sunil.kovvuri@gmail.com>
On Thu, Nov 8, 2018 at 7:37 PM <sunil.kovvuri@gmail.com> wrote:
> @@ -666,4 +668,20 @@ struct npc_mcam_unmap_counter_req {
> u8 all; /* Unmap all entries using this counter ? */
> };
>
> +struct npc_mcam_alloc_and_write_entry_req {
> + struct mbox_msghdr hdr;
> + struct mcam_entry entry_data;
> + u16 ref_entry;
> + u8 priority; /* Lower or higher w.r.t ref_entry */
> + u8 intf; /* Rx or Tx interface */
> + u8 enable_entry;/* Enable this MCAM entry ? */
> + u8 alloc_cntr; /* Allocate counter and map ? */
> +};
I noticed that this structure requires padding at the end because
struct mbox_msghdr has a 32-bit alignment requirement. For
data structures in an interface, I'd recommend avoiding that kind
of padding and adding reserved fields or widening the types
accordingly.
I also noticed a similar problem in struct mbox_msghdr. Maybe
use the 'pahole' tool to check for this kind of padding in the
API structures.
Arnd
^ permalink raw reply
* [PATCH bpf-next v4 10/13] tools/bpf: do not use pahole if clang/llvm can generate BTF sections
From: Yonghong Song @ 2018-11-08 20:37 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: kernel-team
Add additional checks in tools/testing/selftests/bpf and
samples/bpf such that if clang/llvm compiler can generate
BTF sections, do not use pahole.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
samples/bpf/Makefile | 8 ++++++++
tools/testing/selftests/bpf/Makefile | 8 ++++++++
2 files changed, 16 insertions(+)
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index be0a961450bc..35444f4a846b 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -208,12 +208,20 @@ endif
BTF_LLC_PROBE := $(shell $(LLC) -march=bpf -mattr=help 2>&1 | grep dwarfris)
BTF_PAHOLE_PROBE := $(shell $(BTF_PAHOLE) --help 2>&1 | grep BTF)
BTF_OBJCOPY_PROBE := $(shell $(LLVM_OBJCOPY) --help 2>&1 | grep -i 'usage.*llvm')
+BTF_LLVM_PROBE := $(shell echo "int main() { return 0; }" | \
+ $(CLANG) -target bpf -O2 -g -c -x c - -o ./llvm_btf_verify.o; \
+ readelf -S ./llvm_btf_verify.o | grep BTF; \
+ /bin/rm -f ./llvm_btf_verify.o)
+ifneq ($(BTF_LLVM_PROBE),)
+ EXTRA_CFLAGS += -g
+else
ifneq ($(and $(BTF_LLC_PROBE),$(BTF_PAHOLE_PROBE),$(BTF_OBJCOPY_PROBE)),)
EXTRA_CFLAGS += -g
LLC_FLAGS += -mattr=dwarfris
DWARF2BTF = y
endif
+endif
# Trick to allow make to be run from this directory
all:
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e39dfb4e7970..63a321894419 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -124,7 +124,14 @@ $(OUTPUT)/test_stack_map.o: test_queue_stack_map.h
BTF_LLC_PROBE := $(shell $(LLC) -march=bpf -mattr=help 2>&1 | grep dwarfris)
BTF_PAHOLE_PROBE := $(shell $(BTF_PAHOLE) --help 2>&1 | grep BTF)
BTF_OBJCOPY_PROBE := $(shell $(LLVM_OBJCOPY) --help 2>&1 | grep -i 'usage.*llvm')
+BTF_LLVM_PROBE := $(shell echo "int main() { return 0; }" | \
+ $(CLANG) -target bpf -O2 -g -c -x c - -o ./llvm_btf_verify.o; \
+ readelf -S ./llvm_btf_verify.o | grep BTF; \
+ /bin/rm -f ./llvm_btf_verify.o)
+ifneq ($(BTF_LLVM_PROBE),)
+ CLANG_FLAGS += -g
+else
ifneq ($(BTF_LLC_PROBE),)
ifneq ($(BTF_PAHOLE_PROBE),)
ifneq ($(BTF_OBJCOPY_PROBE),)
@@ -134,6 +141,7 @@ ifneq ($(BTF_OBJCOPY_PROBE),)
endif
endif
endif
+endif
$(OUTPUT)/%.o: %.c
$(CLANG) $(CLANG_FLAGS) \
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next v4 11/13] tools/bpf: refactor to implement btf_get_from_id() in lib/bpf
From: Yonghong Song @ 2018-11-08 20:37 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: kernel-team
In-Reply-To: <20181108203740.3139881-1-yhs@fb.com>
The function get_btf() is implemented in tools/bpf/bpftool/map.c
to get a btf structure given a map_info. This patch
refactored this function to be function btf_get_from_id()
in tools/lib/bpf so that it can be used later.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/bpf/bpftool/map.c | 68 ++--------------------------------------
tools/lib/bpf/btf.c | 69 +++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/btf.h | 1 +
3 files changed, 72 insertions(+), 66 deletions(-)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 7bf38f0e152e..1b8a75fa0471 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -215,70 +215,6 @@ static int do_dump_btf(const struct btf_dumper *d,
return ret;
}
-static int get_btf(struct bpf_map_info *map_info, struct btf **btf)
-{
- struct bpf_btf_info btf_info = { 0 };
- __u32 len = sizeof(btf_info);
- __u32 last_size;
- int btf_fd;
- void *ptr;
- int err;
-
- err = 0;
- *btf = NULL;
- btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
- if (btf_fd < 0)
- return 0;
-
- /* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
- * let's start with a sane default - 4KiB here - and resize it only if
- * bpf_obj_get_info_by_fd() needs a bigger buffer.
- */
- btf_info.btf_size = 4096;
- last_size = btf_info.btf_size;
- ptr = malloc(last_size);
- if (!ptr) {
- err = -ENOMEM;
- goto exit_free;
- }
-
- bzero(ptr, last_size);
- btf_info.btf = ptr_to_u64(ptr);
- err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
-
- if (!err && btf_info.btf_size > last_size) {
- void *temp_ptr;
-
- last_size = btf_info.btf_size;
- temp_ptr = realloc(ptr, last_size);
- if (!temp_ptr) {
- err = -ENOMEM;
- goto exit_free;
- }
- ptr = temp_ptr;
- bzero(ptr, last_size);
- btf_info.btf = ptr_to_u64(ptr);
- err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
- }
-
- if (err || btf_info.btf_size > last_size) {
- err = errno;
- goto exit_free;
- }
-
- *btf = btf__new((__u8 *)btf_info.btf, btf_info.btf_size, NULL);
- if (IS_ERR(*btf)) {
- err = PTR_ERR(*btf);
- *btf = NULL;
- }
-
-exit_free:
- close(btf_fd);
- free(ptr);
-
- return err;
-}
-
static json_writer_t *get_btf_writer(void)
{
json_writer_t *jw = jsonw_new(stdout);
@@ -765,7 +701,7 @@ static int do_dump(int argc, char **argv)
prev_key = NULL;
- err = get_btf(&info, &btf);
+ err = btf_get_from_id(info.btf_id, &btf);
if (err) {
p_err("failed to get btf");
goto exit_free;
@@ -909,7 +845,7 @@ static int do_lookup(int argc, char **argv)
}
/* here means bpf_map_lookup_elem() succeeded */
- err = get_btf(&info, &btf);
+ err = btf_get_from_id(info.btf_id, &btf);
if (err) {
p_err("failed to get btf");
goto exit_free;
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 11579ed4ef6c..2a914ab65cbc 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -49,6 +49,11 @@ struct bpf_func_info_min {
__u32 type_id;
};
+static inline __u64 ptr_to_u64(const void *ptr)
+{
+ return (__u64) (unsigned long) ptr;
+}
+
static int btf_add_type(struct btf *btf, struct btf_type *t)
{
if (btf->types_size - btf->nr_types < 2) {
@@ -409,6 +414,70 @@ const char *btf__name_by_offset(const struct btf *btf, __u32 offset)
return NULL;
}
+int btf_get_from_id(__u32 id, struct btf **btf)
+{
+ struct bpf_btf_info btf_info = { 0 };
+ __u32 len = sizeof(btf_info);
+ __u32 last_size;
+ int btf_fd;
+ void *ptr;
+ int err;
+
+ err = 0;
+ *btf = NULL;
+ btf_fd = bpf_btf_get_fd_by_id(id);
+ if (btf_fd < 0)
+ return 0;
+
+ /* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
+ * let's start with a sane default - 4KiB here - and resize it only if
+ * bpf_obj_get_info_by_fd() needs a bigger buffer.
+ */
+ btf_info.btf_size = 4096;
+ last_size = btf_info.btf_size;
+ ptr = malloc(last_size);
+ if (!ptr) {
+ err = -ENOMEM;
+ goto exit_free;
+ }
+
+ bzero(ptr, last_size);
+ btf_info.btf = ptr_to_u64(ptr);
+ err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+
+ if (!err && btf_info.btf_size > last_size) {
+ void *temp_ptr;
+
+ last_size = btf_info.btf_size;
+ temp_ptr = realloc(ptr, last_size);
+ if (!temp_ptr) {
+ err = -ENOMEM;
+ goto exit_free;
+ }
+ ptr = temp_ptr;
+ bzero(ptr, last_size);
+ btf_info.btf = ptr_to_u64(ptr);
+ err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+ }
+
+ if (err || btf_info.btf_size > last_size) {
+ err = errno;
+ goto exit_free;
+ }
+
+ *btf = btf__new((__u8 *)btf_info.btf, btf_info.btf_size, NULL);
+ if (IS_ERR(*btf)) {
+ err = PTR_ERR(*btf);
+ *btf = NULL;
+ }
+
+exit_free:
+ close(btf_fd);
+ free(ptr);
+
+ return err;
+}
+
static int btf_ext_validate_func_info(const void *finfo, __u32 size,
btf_print_fn_t err_log)
{
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 578171e8cb26..386b2ffc32a3 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -69,6 +69,7 @@ LIBBPF_API __s64 btf__resolve_size(const struct btf *btf, __u32 type_id);
LIBBPF_API int btf__resolve_type(const struct btf *btf, __u32 type_id);
LIBBPF_API int btf__fd(const struct btf *btf);
LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
+LIBBPF_API int btf_get_from_id(__u32 id, struct btf **btf);
struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log);
void btf_ext__free(struct btf_ext *btf_ext);
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next v4 13/13] tools/bpf: bpftool: add support for func types
From: Yonghong Song @ 2018-11-08 20:37 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: kernel-team
In-Reply-To: <20181108203740.3139881-1-yhs@fb.com>
This patch added support to print function signature
if btf func_info is available. Note that ksym
now uses function name instead of prog_name as
prog_name has a limit of 16 bytes including
ending '\0'.
The following is a sample output for selftests
test_btf with file test_btf_haskv.o for translated insns
and jited insns respectively.
$ bpftool prog dump xlated id 1
int _dummy_tracepoint(struct dummy_tracepoint_args * arg):
0: (85) call pc+2#bpf_prog_2dcecc18072623fc_test_long_fname_1
1: (b7) r0 = 0
2: (95) exit
int test_long_fname_1(struct dummy_tracepoint_args * arg):
3: (85) call pc+1#bpf_prog_89d64e4abf0f0126_test_long_fname_2
4: (95) exit
int test_long_fname_2(struct dummy_tracepoint_args * arg):
5: (b7) r2 = 0
6: (63) *(u32 *)(r10 -4) = r2
7: (79) r1 = *(u64 *)(r1 +8)
...
22: (07) r1 += 1
23: (63) *(u32 *)(r0 +4) = r1
24: (95) exit
$ bpftool prog dump jited id 1
int _dummy_tracepoint(struct dummy_tracepoint_args * arg):
bpf_prog_b07ccb89267cf242__dummy_tracepoint:
0: push %rbp
1: mov %rsp,%rbp
......
3c: add $0x28,%rbp
40: leaveq
41: retq
int test_long_fname_1(struct dummy_tracepoint_args * arg):
bpf_prog_2dcecc18072623fc_test_long_fname_1:
0: push %rbp
1: mov %rsp,%rbp
......
3a: add $0x28,%rbp
3e: leaveq
3f: retq
int test_long_fname_2(struct dummy_tracepoint_args * arg):
bpf_prog_89d64e4abf0f0126_test_long_fname_2:
0: push %rbp
1: mov %rsp,%rbp
......
80: add $0x28,%rbp
84: leaveq
85: retq
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/bpf/bpftool/btf_dumper.c | 98 +++++++++++++++++++++++++++++++
tools/bpf/bpftool/main.h | 2 +
tools/bpf/bpftool/prog.c | 56 ++++++++++++++++++
tools/bpf/bpftool/xlated_dumper.c | 33 +++++++++++
tools/bpf/bpftool/xlated_dumper.h | 3 +
5 files changed, 192 insertions(+)
diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
index 55bc512a1831..64b63d53a143 100644
--- a/tools/bpf/bpftool/btf_dumper.c
+++ b/tools/bpf/bpftool/btf_dumper.c
@@ -249,3 +249,101 @@ int btf_dumper_type(const struct btf_dumper *d, __u32 type_id,
{
return btf_dumper_do_type(d, type_id, 0, data);
}
+
+#define BTF_PRINT_ARG(...) \
+ do { \
+ pos += snprintf(func_sig + pos, size - pos, \
+ __VA_ARGS__); \
+ if (pos >= size) \
+ return -1; \
+ } while (0)
+#define BTF_PRINT_TYPE(type) \
+ do { \
+ pos = __btf_dumper_type_only(btf, type, func_sig, \
+ pos, size); \
+ if (pos == -1) \
+ return -1; \
+ } while (0)
+
+static int __btf_dumper_type_only(struct btf *btf, __u32 type_id,
+ char *func_sig, int pos, int size)
+{
+ const struct btf_type *t = btf__type_by_id(btf, type_id);
+ const struct btf_array *array;
+ int i, vlen;
+
+ switch (BTF_INFO_KIND(t->info)) {
+ case BTF_KIND_INT:
+ BTF_PRINT_ARG("%s ", btf__name_by_offset(btf, t->name_off));
+ break;
+ case BTF_KIND_STRUCT:
+ BTF_PRINT_ARG("struct %s ",
+ btf__name_by_offset(btf, t->name_off));
+ break;
+ case BTF_KIND_UNION:
+ BTF_PRINT_ARG("union %s ",
+ btf__name_by_offset(btf, t->name_off));
+ break;
+ case BTF_KIND_ENUM:
+ BTF_PRINT_ARG("enum %s ",
+ btf__name_by_offset(btf, t->name_off));
+ break;
+ case BTF_KIND_ARRAY:
+ array = (struct btf_array *)(t + 1);
+ BTF_PRINT_TYPE(array->type);
+ BTF_PRINT_ARG("[%d]", array->nelems);
+ break;
+ case BTF_KIND_PTR:
+ BTF_PRINT_TYPE(t->type);
+ BTF_PRINT_ARG("* ");
+ break;
+ case BTF_KIND_UNKN:
+ case BTF_KIND_FWD:
+ case BTF_KIND_TYPEDEF:
+ return -1;
+ case BTF_KIND_VOLATILE:
+ BTF_PRINT_ARG("volatile ");
+ BTF_PRINT_TYPE(t->type);
+ break;
+ case BTF_KIND_CONST:
+ BTF_PRINT_ARG("const ");
+ BTF_PRINT_TYPE(t->type);
+ break;
+ case BTF_KIND_RESTRICT:
+ BTF_PRINT_ARG("restrict ");
+ BTF_PRINT_TYPE(t->type);
+ break;
+ case BTF_KIND_FUNC:
+ BTF_PRINT_TYPE(t->type);
+ BTF_PRINT_ARG("%s(", btf__name_by_offset(btf, t->name_off));
+ vlen = BTF_INFO_VLEN(t->info);
+ for (i = 0; i < vlen; i++) {
+ struct btf_param *arg = &((struct btf_param *)(t + 1))[i];
+
+ if (i)
+ BTF_PRINT_ARG(", ");
+ BTF_PRINT_TYPE(arg->type);
+ BTF_PRINT_ARG("%s", btf__name_by_offset(btf, arg->name_off));
+ }
+ BTF_PRINT_ARG(")");
+ break;
+ default:
+ return -1;
+ }
+
+ return pos;
+}
+
+void btf_dumper_type_only(struct btf *btf, __u32 type_id, char *func_sig,
+ int size)
+{
+ int err;
+
+ func_sig[0] = '\0';
+ if (!btf)
+ return;
+
+ err = __btf_dumper_type_only(btf, type_id, func_sig, 0, size);
+ if (err < 0)
+ func_sig[0] = '\0';
+}
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 28322ace2856..c72129fb4e66 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -170,6 +170,8 @@ struct btf_dumper {
*/
int btf_dumper_type(const struct btf_dumper *d, __u32 type_id,
const void *data);
+void btf_dumper_type_only(struct btf *btf, __u32 func_type_id, char *func_only,
+ int size);
struct nlattr;
struct ifinfomsg;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 5302ee282409..27df912aa45a 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -47,6 +47,7 @@
#include <linux/err.h>
#include <bpf.h>
+#include <btf.h>
#include <libbpf.h>
#include "cfg.h"
@@ -450,14 +451,19 @@ static int do_dump(int argc, char **argv)
struct bpf_prog_info info = {};
unsigned int *func_lens = NULL;
const char *disasm_opt = NULL;
+ unsigned int finfo_rec_size;
unsigned int nr_func_ksyms;
unsigned int nr_func_lens;
struct dump_data dd = {};
__u32 len = sizeof(info);
+ struct btf *btf = NULL;
+ void *func_info = NULL;
+ unsigned int finfo_cnt;
unsigned int buf_size;
char *filepath = NULL;
bool opcodes = false;
bool visual = false;
+ char func_sig[1024];
unsigned char *buf;
__u32 *member_len;
__u64 *member_ptr;
@@ -547,6 +553,17 @@ static int do_dump(int argc, char **argv)
}
}
+ finfo_cnt = info.func_info_cnt;
+ finfo_rec_size = info.func_info_rec_size;
+ if (finfo_cnt && finfo_rec_size) {
+ func_info = malloc(finfo_cnt * finfo_rec_size);
+ if (!func_info) {
+ p_err("mem alloc failed");
+ close(fd);
+ goto err_free;
+ }
+ }
+
memset(&info, 0, sizeof(info));
*member_ptr = ptr_to_u64(buf);
@@ -555,6 +572,9 @@ static int do_dump(int argc, char **argv)
info.nr_jited_ksyms = nr_func_ksyms;
info.jited_func_lens = ptr_to_u64(func_lens);
info.nr_jited_func_lens = nr_func_lens;
+ info.func_info_cnt = finfo_cnt;
+ info.func_info_rec_size = finfo_rec_size;
+ info.func_info = ptr_to_u64(func_info);
err = bpf_obj_get_info_by_fd(fd, &info, &len);
close(fd);
@@ -578,6 +598,18 @@ static int do_dump(int argc, char **argv)
goto err_free;
}
+ if (info.func_info_cnt != finfo_cnt) {
+ p_err("incorrect func_info_cnt %d vs. expected %d",
+ info.func_info_cnt, finfo_cnt);
+ goto err_free;
+ }
+
+ if (info.func_info_rec_size != finfo_rec_size) {
+ p_err("incorrect func_info_rec_size %d vs. expected %d",
+ info.func_info_rec_size, finfo_rec_size);
+ goto err_free;
+ }
+
if ((member_len == &info.jited_prog_len &&
info.jited_prog_insns == 0) ||
(member_len == &info.xlated_prog_len &&
@@ -586,6 +618,11 @@ static int do_dump(int argc, char **argv)
goto err_free;
}
+ if (info.btf_id && btf_get_from_id(info.btf_id, &btf)) {
+ p_err("failed to get btf");
+ goto err_free;
+ }
+
if (filepath) {
fd = open(filepath, O_WRONLY | O_CREAT | O_TRUNC, 0600);
if (fd < 0) {
@@ -618,6 +655,7 @@ static int do_dump(int argc, char **argv)
if (info.nr_jited_func_lens && info.jited_func_lens) {
struct kernel_sym *sym = NULL;
+ struct bpf_func_info *record;
char sym_name[SYM_MAX_NAME];
unsigned char *img = buf;
__u64 *ksyms = NULL;
@@ -644,12 +682,25 @@ static int do_dump(int argc, char **argv)
strcpy(sym_name, "unknown");
}
+ if (func_info) {
+ record = func_info + i * finfo_rec_size;
+ btf_dumper_type_only(btf, record->type_id,
+ func_sig,
+ sizeof(func_sig));
+ }
+
if (json_output) {
jsonw_start_object(json_wtr);
+ if (func_info && func_sig[0] != '\0') {
+ jsonw_name(json_wtr, "proto");
+ jsonw_string(json_wtr, func_sig);
+ }
jsonw_name(json_wtr, "name");
jsonw_string(json_wtr, sym_name);
jsonw_name(json_wtr, "insns");
} else {
+ if (func_info && func_sig[0] != '\0')
+ printf("%s:\n", func_sig);
printf("%s:\n", sym_name);
}
@@ -678,6 +729,9 @@ static int do_dump(int argc, char **argv)
kernel_syms_load(&dd);
dd.nr_jited_ksyms = info.nr_jited_ksyms;
dd.jited_ksyms = (__u64 *) info.jited_ksyms;
+ dd.btf = btf;
+ dd.func_info = func_info;
+ dd.finfo_rec_size = finfo_rec_size;
if (json_output)
dump_xlated_json(&dd, buf, *member_len, opcodes);
@@ -689,12 +743,14 @@ static int do_dump(int argc, char **argv)
free(buf);
free(func_ksyms);
free(func_lens);
+ free(func_info);
return 0;
err_free:
free(buf);
free(func_ksyms);
free(func_lens);
+ free(func_info);
return -1;
}
diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c
index 3284759df98a..e06ac0286a75 100644
--- a/tools/bpf/bpftool/xlated_dumper.c
+++ b/tools/bpf/bpftool/xlated_dumper.c
@@ -242,11 +242,15 @@ void dump_xlated_json(struct dump_data *dd, void *buf, unsigned int len,
.cb_imm = print_imm,
.private_data = dd,
};
+ struct bpf_func_info *record;
struct bpf_insn *insn = buf;
+ struct btf *btf = dd->btf;
bool double_insn = false;
+ char func_sig[1024];
unsigned int i;
jsonw_start_array(json_wtr);
+ record = dd->func_info;
for (i = 0; i < len / sizeof(*insn); i++) {
if (double_insn) {
double_insn = false;
@@ -255,6 +259,20 @@ void dump_xlated_json(struct dump_data *dd, void *buf, unsigned int len,
double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
jsonw_start_object(json_wtr);
+
+ if (btf && record) {
+ if (record->insn_offset == i) {
+ btf_dumper_type_only(btf, record->type_id,
+ func_sig,
+ sizeof(func_sig));
+ if (func_sig[0] != '\0') {
+ jsonw_name(json_wtr, "proto");
+ jsonw_string(json_wtr, func_sig);
+ }
+ record = (void *)record + dd->finfo_rec_size;
+ }
+ }
+
jsonw_name(json_wtr, "disasm");
print_bpf_insn(&cbs, insn + i, true);
@@ -297,16 +315,31 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
.cb_imm = print_imm,
.private_data = dd,
};
+ struct bpf_func_info *record;
struct bpf_insn *insn = buf;
+ struct btf *btf = dd->btf;
bool double_insn = false;
+ char func_sig[1024];
unsigned int i;
+ record = dd->func_info;
for (i = 0; i < len / sizeof(*insn); i++) {
if (double_insn) {
double_insn = false;
continue;
}
+ if (btf && record) {
+ if (record->insn_offset == i) {
+ btf_dumper_type_only(btf, record->type_id,
+ func_sig,
+ sizeof(func_sig));
+ if (func_sig[0] != '\0')
+ printf("%s:\n", func_sig);
+ record = (void *)record + dd->finfo_rec_size;
+ }
+ }
+
double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
printf("% 4d: ", i);
diff --git a/tools/bpf/bpftool/xlated_dumper.h b/tools/bpf/bpftool/xlated_dumper.h
index 33d86e2b369b..aec31723e1e5 100644
--- a/tools/bpf/bpftool/xlated_dumper.h
+++ b/tools/bpf/bpftool/xlated_dumper.h
@@ -51,6 +51,9 @@ struct dump_data {
__u32 sym_count;
__u64 *jited_ksyms;
__u32 nr_jited_ksyms;
+ struct btf *btf;
+ void *func_info;
+ __u32 finfo_rec_size;
char scratch_buff[SYM_MAX_NAME + 8];
};
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox