* Re: [PATCH net] net: sched: act_tunnel_key: fix NULL pointer dereference during init
From: Davide Caratti @ 2019-02-25 15:36 UTC (permalink / raw)
To: Vlad Buslov, netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, wenxu, roid
In-Reply-To: <20190225152827.8741-1-vladbu@mellanox.com>
On Mon, 2019-02-25 at 17:28 +0200, Vlad Buslov wrote:
> Metadata pointer is only initialized for action TCA_TUNNEL_KEY_ACT_SET, but
> it is unconditionally dereferenced in tunnel_key_init() error handler.
> Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
>
> Fixes: ee28bb56ac5b ("net/sched: fix memory leak in act_tunnel_key_init()")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> ---
> net/sched/act_tunnel_key.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 8b43fe0130f7..3f943de9a2c9 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -377,7 +377,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> return ret;
>
> release_tun_meta:
> - dst_release(&metadata->dst);
> + if (metadata)
> + dst_release(&metadata->dst);
>
> err_out:
> if (exists)
Reviewed-by: Davide Caratti <dcaratti@redhat.com>
^ permalink raw reply
* [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Vlad Buslov @ 2019-02-25 15:38 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
at least once is unreliable. Some classifiers set 'stop' flag on error
before calling walker callback, other classifiers used to call it with NULL
filter pointer when empty. In order to prevent further regressions, extend
tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
tcf_walker->fn() implementation that is used to check if classifier has
filters configured.
Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
---
include/net/pkt_cls.h | 1 +
net/sched/cls_api.c | 13 +++++++++----
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 232f801f2a21..422dd8800478 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -17,6 +17,7 @@ struct tcf_walker {
int stop;
int skip;
int count;
+ bool nonempty;
unsigned long cookie;
int (*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
};
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index e2c888961379..3543be31d400 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
tcf_proto_destroy(tp, rtnl_held, extack);
}
-static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
+static int walker_check_empty(struct tcf_proto *tp, void *d,
+ struct tcf_walker *arg)
{
- return -1;
+ if (tp) {
+ arg->nonempty = true;
+ return -1;
+ }
+ return 0;
}
static bool tcf_proto_is_empty(struct tcf_proto *tp, bool rtnl_held)
{
- struct tcf_walker walker = { .fn = walker_noop, };
+ struct tcf_walker walker = { .fn = walker_check_empty, };
if (tp->ops->walk) {
tp->ops->walk(tp, &walker, rtnl_held);
- return !walker.stop;
+ return !walker.nonempty;
}
return true;
}
--
2.13.6
^ permalink raw reply related
* Re: [PATCH bpf] bpf: properly check TCP_CONGESTION optlen
From: Eric Dumazet @ 2019-02-25 15:43 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Eric Dumazet, Alexei Starovoitov, netdev,
Martin KaFai Lau, Song Liu, Yonghong Song, bpf, Lawrence Brakmo
In-Reply-To: <739fbe32-d019-4d97-c902-2155a15c58b5@iogearbox.net>
On Mon, Feb 25, 2019 at 3:07 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/24/2019 12:11 AM, Alexei Starovoitov wrote:
> > On Sat, Feb 23, 2019 at 12:48:53PM -0800, Eric Dumazet wrote:
> >> On 02/23/2019 12:38 PM, Alexei Starovoitov wrote:
> >>> On Sat, Feb 23, 2019 at 11:07:09AM -0800, Eric Dumazet wrote:
> >>>> If caller of bpf_setsockopt() is silly passing a negative optlen
> >>>> bad things happen.
> >>>>
> >>>> Fixes: 91b5b21c7c16 ("bpf: Add support for changing congestion control")
> >>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
> >>>> Cc: Lawrence Brakmo <brakmo@fb.com>
> >>>> ---
> >>>> net/core/filter.c | 5 +++--
> >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/net/core/filter.c b/net/core/filter.c
> >>>> index f7d0004fc16096eb42ece3a6acf645540ee2326b..6a5d89464168f2f35f43986c1dbc0446c9390a3c 100644
> >>>> --- a/net/core/filter.c
> >>>> +++ b/net/core/filter.c
> >>>> @@ -4194,8 +4194,9 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
> >>>> char name[TCP_CA_NAME_MAX];
> >>>> bool reinit = bpf_sock->op > BPF_SOCK_OPS_NEEDS_ECN;
> >>>>
> >>>> - strncpy(name, optval, min_t(long, optlen,
> >>>> - TCP_CA_NAME_MAX-1));
> >>>> + if (optlen < 0)
> >>>> + return -EINVAL;
> >>>> + strncpy(name, optval, min(optlen, TCP_CA_NAME_MAX - 1));
> >>>
> >>> Unnecessary.
> >>> The verifier guarantees that optlen > 0 because
> >>> static const struct bpf_func_proto bpf_setsockopt_proto = {
> >>> .func = bpf_setsockopt,
> >>> ...
> >>> .arg5_type = ARG_CONST_SIZE,
> >>> };
> >>
> >> Even on 32bit kernel ?
> >>
> >> The suspect thing to me is the min_t(long, ....)
> >>
> >> optlen is an integer, why do we need to promote to a long ?
> >
> > I think the code is actually fine as-is.
> > I bet it was copy pasted from do_tcp_setsockopt
> > where similar min_t(long) is used to match strncpy_from_user() declaration.
> > Here min_t(long) or min_t(int) or min() doesn't matter.
> > I would keep it as-is to avoid noisy patches.
>
> Max allowed input from verifier should be BPF_MAX_VAR_SIZ which is 1 << 29,
> but I totally agree that the bpf_setsockopt() and bpf_getsockopt() signature
> should change into u32 optlen as it's just confusing otherwise, same with the
> long in strncpy(). Agree with Alexei that this might have been a copy-paste
> kind of thing. Lawrence can probably clarify best?
>
> The same wrong assumption is in commit 1e215300f138 ("bpf: add TCP_SAVE_SYN/
> TCP_SAVED_SYN options for bpf_(set|get)sockopt") which tests for optlen <= 0.
> Neither can it be negative nor zero here due to ARG_CONST_SIZE. Given it's
> also confusing others, cleanup might still be worth considering. Maybe Lawrence
> can spin this into one of his next patches, if noone else gets to it first.
Yes, and considering the err_clear: error path does
memset(optval, 0, optlen);
This is even more confusing to think that optlen could be negative or
out of bound ;)
^ permalink raw reply
* [PATCH ethtool V2] ethtool: Add support for 200Gbps (50Gbps per lane) link mode
From: Tariq Toukan @ 2019-02-25 15:44 UTC (permalink / raw)
To: John W. Linville
Cc: netdev, Eran Ben Elisha, alaa, Andrew Lunn, Michal Kubecek,
Aya Levin, Tariq Toukan
From: Aya Levin <ayal@mellanox.com>
Introduce 50Gbps per lane link modes and 200Gbps speed, update print
functions and initialization functions accordingly.
In addition, update related man page accordingly.
Signed-off-by: Aya Levin <ayal@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
ethtool-copy.h | 18 +++++++++++++++++-
ethtool.8.in | 15 +++++++++++++++
ethtool.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 77 insertions(+), 1 deletion(-)
V2:
- Removed wrong and unneeded initialization of adv_bit.
- Removed an addition in ethtool-copy.h that does not exist in kernel.
diff --git a/ethtool-copy.h b/ethtool-copy.h
index 6bfbb85f9402..120407604232 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -1455,6 +1455,21 @@ enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_FEC_NONE_BIT = 49,
ETHTOOL_LINK_MODE_FEC_RS_BIT = 50,
ETHTOOL_LINK_MODE_FEC_BASER_BIT = 51,
+ ETHTOOL_LINK_MODE_50000baseKR_Full_BIT = 52,
+ ETHTOOL_LINK_MODE_50000baseSR_Full_BIT = 53,
+ ETHTOOL_LINK_MODE_50000baseCR_Full_BIT = 54,
+ ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT = 55,
+ ETHTOOL_LINK_MODE_50000baseDR_Full_BIT = 56,
+ ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT = 57,
+ ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT = 58,
+ ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT = 59,
+ ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT = 60,
+ ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT = 61,
+ ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT = 62,
+ ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT = 63,
+ ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64,
+ ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT = 65,
+ ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66,
/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
* 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
@@ -1463,7 +1478,7 @@ enum ethtool_link_mode_bit_indices {
*/
__ETHTOOL_LINK_MODE_LAST
- = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
+ = ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT,
};
#define __ETHTOOL_LINK_MODE_LEGACY_MASK(base_name) \
@@ -1571,6 +1586,7 @@ enum ethtool_link_mode_bit_indices {
#define SPEED_50000 50000
#define SPEED_56000 56000
#define SPEED_100000 100000
+#define SPEED_200000 200000
#define SPEED_UNKNOWN -1
diff --git a/ethtool.8.in b/ethtool.8.in
index 5a26cff5fb33..64ce0711ad5f 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -650,6 +650,11 @@ lB l lB.
0x400000000 50000baseCR2 Full
0x800000000 50000baseKR2 Full
0x10000000000 50000baseSR2 Full
+0x10000000000000 50000baseKR Full
+0x20000000000000 50000baseSR Full
+0x40000000000000 50000baseCR Full
+0x80000000000000 50000baseLR_ER_FR Full
+0x100000000000000 50000baseDR Full
0x8000000 56000baseKR4 Full
0x10000000 56000baseCR4 Full
0x20000000 56000baseSR4 Full
@@ -658,6 +663,16 @@ lB l lB.
0x2000000000 100000baseSR4 Full
0x4000000000 100000baseCR4 Full
0x8000000000 100000baseLR4_ER4 Full
+0x200000000000000 100000baseKR2 Full
+0x400000000000000 100000baseSR2 Full
+0x800000000000000 100000baseCR2 Full
+0x1000000000000000 100000baseLR2_ER2_FR2 Full
+0x2000000000000000 100000baseDR2 Full
+0x4000000000000000 200000baseKR4 Full
+0x8000000000000000 200000baseSR4 Full
+0x10000000000000000 200000baseLR4_ER4_FR4 Full
+0x20000000000000000 200000baseDR4 Full
+0x40000000000000000 200000baseCR4 Full
.TE
.TP
.BI phyad \ N
diff --git a/ethtool.c b/ethtool.c
index fb4c0886ca84..c36aafd5bb3a 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -530,6 +530,21 @@ static void init_global_link_mode_masks(void)
ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseDR_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT,
};
static const enum ethtool_link_mode_bit_indices
additional_advertised_flags_bits[] = {
@@ -689,6 +704,36 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
"2500baseT/Full" },
{ 0, ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
"5000baseT/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
+ "50000baseKR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
+ "50000baseSR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
+ "50000baseCR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
+ "50000baseLR_ER_FR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseDR_Full_BIT,
+ "50000baseDR/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT,
+ "100000baseKR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT,
+ "100000baseSR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT,
+ "100000baseCR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT,
+ "100000baseLR2_ER2_FR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT,
+ "100000baseDR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT,
+ "200000baseKR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT,
+ "200000baseSR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT,
+ "200000baseLR4_ER4_FR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT,
+ "200000baseDR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT,
+ "200000baseCR4/Full" },
};
int indent;
int did1, new_line_pend, i;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next] net: sched: don't release block->lock when dumping chains
From: Vlad Buslov @ 2019-02-25 15:45 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
Function tc_dump_chain() obtains and releases block->lock on each iteration
of its inner loop that dumps all chains on block. Outputting chain template
info is fast operation so locking/unlocking mutex multiple times is an
overhead when lock is highly contested. Modify tc_dump_chain() to only
obtain block->lock once and dump all chains without releasing it.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/cls_api.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 3543be31d400..5c6a9baa389f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2996,12 +2996,12 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
/* called with RTNL */
static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
{
- struct tcf_chain *chain, *chain_prev;
struct net *net = sock_net(skb->sk);
struct nlattr *tca[TCA_MAX + 1];
struct Qdisc *q = NULL;
struct tcf_block *block;
struct tcmsg *tcm = nlmsg_data(cb->nlh);
+ struct tcf_chain *chain;
long index_start;
long index;
u32 parent;
@@ -3064,11 +3064,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
index_start = cb->args[0];
index = 0;
- for (chain = __tcf_get_next_chain(block, NULL);
- chain;
- chain_prev = chain,
- chain = __tcf_get_next_chain(block, chain),
- tcf_chain_put(chain_prev)) {
+ mutex_lock(&block->lock);
+ list_for_each_entry(chain, &block->chain_list, list) {
if ((tca[TCA_CHAIN] &&
nla_get_u32(tca[TCA_CHAIN]) != chain->index))
continue;
@@ -3076,17 +3073,18 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
index++;
continue;
}
+ if (tcf_chain_held_by_acts_only(chain))
+ continue;
err = tc_chain_fill_node(chain->tmplt_ops, chain->tmplt_priv,
chain->index, net, skb, block,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
RTM_NEWCHAIN);
- if (err <= 0) {
- tcf_chain_put(chain);
+ if (err <= 0)
break;
- }
index++;
}
+ mutex_unlock(&block->lock);
if (tcm->tcm_ifindex == TCM_IFINDEX_MAGIC_BLOCK)
tcf_block_refcnt_put(block, true);
--
2.13.6
^ permalink raw reply related
* Re: [PATCH net-next] tcp: remove unused parameter of tcp_sacktag_bsearch()
From: Eric Dumazet @ 2019-02-25 15:47 UTC (permalink / raw)
To: Taehee Yoo; +Cc: David Miller, netdev, Yuchung Cheng
In-Reply-To: <20190225094233.16231-1-ap420073@gmail.com>
On Mon, Feb 25, 2019 at 1:42 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> parameter state in the tcp_sacktag_bsearch() is not used.
> So, it can be removed.
Yes, this is a nice cleanup that should have followed commit
737ff314563ca27f044f9a3a041e9d42491ef7ce ("tcp: use sequence distance
to detect reordering")
since it removed fack_count
Signed-off-by: Eric Dumazet <edumazet@google.com>
Thanks.
^ permalink raw reply
* Re: [PATCH] tun: fix blocking read
From: Eric Dumazet @ 2019-02-25 15:53 UTC (permalink / raw)
To: David Miller, mail; +Cc: netdev, linux-kernel
In-Reply-To: <20190224.221236.905722532971991928.davem@davemloft.net>
On 02/24/2019 10:12 PM, David Miller wrote:
> From: Timur Celik <mail@timurcelik.de>
> Date: Sat, 23 Feb 2019 12:53:13 +0100
>
>> This patch moves setting of the current state into the loop. Otherwise
>> the task may end up in a busy wait loop if none of the break conditions
>> are met.
>>
>> Signed-off-by: Timur Celik <mail@timurcelik.de>
>
> Applied and queued up for -stable, thanks.
>
First part of the patch was matching the changelog, however
the second change was not really needed, or should have used
__set_current_state(TASK_RUNNING);
^ permalink raw reply
* [PATCH 0/2] bpf: context casting for tail call and gtrace prog type
From: Kris Van Hees @ 2019-02-25 15:54 UTC (permalink / raw)
To: netdev
The patches in this set are part of an effort to provide support for
tracing tools beyond attaching programs to probes and events and working
with the context data they provide. It is also aimed at avoiding adding
new helpers for every piece of task information that tracers may want to
include in trace data (as was discussed at the Linux Plumbers Conference
BPF mini-conference track last year).
One of the main characteristics of tracers is that a variety of
information can be collected at the time of a probe firing. When using
BPF program to implement actions to be taken when a probe fires, The most
natural source of a large part of this information (task information,
probe data, tracer state) is the context that is associated with a BPF
program. It is also possible to obtain (most of) this information by
means of full access helper calls like probe_read() but that isn't
something you want to make available to an unprivileged user.
So we have two areas where BPF programs can be very useful and powerful:
- BPF programs that are attached to a probe or event, operating on the
context provided by the probe or event
- BPF programs that implement actions to be taken within the context of a
tracing tool
The Linux kernel provides a wealth of probes and events to which we can
attach BPF programs. These event sources do not have any knowledge of the
tracing tool that might be using them. But being able to use them from any
tracing tool is definitely preferable over implementing your own probes and
events. We definitely also do not want to 'teach' all the existing probes
and events about any possible BPF program type that would like to get called
from those probes and events.
So, to illustrate what we're trying to accomplish, consider a kprobe. We
can attach a BPF program to it and it will be called with a 'struct pt_regs'
context. From the side of our tracing tool, we also want information about
the task that triggered the kprobe to fire (beyond what is currently
available through helpers) and we want to be able to access that information
from a BPF program that implements what should happen when the probe fires
(e.g. recording the event and specific data that we are interested in).
The 2nd patch in this set implements a very basic generic tracer program
type BPF_PROG_TYPE_GTRACE) that provides the pt_regs data and select task
data in its context. We cannot attach a program of this type to a kprobe
because that probe supports BPF_PROG_TYPE_KPROBE instead.
The 1st patch in this set implements a mechanism to solve this issue: it
allows a tail-call from one program type to another if the callee type
supports conversion of a caller context into a context for the callee.
So, in the sample, the BPF_PROG_TYPE_GTRACE provides can_cast() and
cast_context() functions that support converting a BPF_PROG_TYPE_KPROBE
context into a BPF_PROG_TYPE_GTRACE context.
The work flow a tracer can use is:
1. The tracer creates a program array map, and inserts one or more programs
of type BPF_PROG_TYPE_GTRACE. These programs implement whatever actions
are to be taken when a specific probe fires. This step must be done first
so that the program array is initialized with the correct program type.
This type needs to be known so that when the calling program is verified,
compatibility checking can be performed.
2. The tracer loads a program of type BPF_PROG_TYPE_KPROBE and attaches it
to the kprobe we're interested in. This program contains a tail-call to
a BPF_PROG_TYPE_GTRACE program in the program array.
3. The kprobe fires and executes our program (of type BPF_PROG_TYPE_KPROBE).
3.1 The program performs whatever operations that we need to have done
at the level of the probe firing.
3.2 The program performs a tail-call into a program from our program array.
3.2.1 The execution of the tail-call instruction causes a call to be
made to a cast_context() function provided by BPF_PROG_TYPE_GTRACE.
This function creates a context structure, and populates it with
task information and copies in the pt_regs data from the context
that was passed to the BPF_PROG_TYPE_KPROBE program.
3.2.2 The new context is assigned to R1 (replacing the original context),
and execution is transferred to the called program.
The implementation is done in such way that existing tail-calls will work
without any change aside from the fact that the verifier is inserting an
instruction right before the tail-call. That instruction simply loads the
BPF program type into R4. This ensures that at the time of the tail-call,
the program type of the calling program can be passed to the cast_context()
function. Knowledge about the program type of an executing program is not
available anywhere and we need to know what context we're trying to convert
from. The function prototype for the (pseudo-)helper bpf_tail_call declares
only 3 arguments so existing code is not affected by this internal use of R4.
Obviously, if there is no conversion function or the conversion is not
supported, the tail-call will fail because that situation is effectively the
same as trying to call a program of an incompatible type.
The goal is to further extend the BPF_PROG_TYPE_GTRACE implementation to
support what tracers commonly need, and I am also looking at ways to further
extend this model to allow more tracer-specific features as well without the
need for adding a BPF program types for every tracer.
Kris
^ permalink raw reply
* [PATCH 1/2] bpf: context casting for tail call
From: Kris Van Hees @ 2019-02-25 15:54 UTC (permalink / raw)
To: netdev
Currently BPF programs are executed with a context that is provided by
code that initiates the execution. Tracing tools that want to make use
of existing probes and events that allow BPF programs to be attached to
them are thus limited to the context information provided by the probe
or event source. Often, more context is needed to allow tracing tools
the ablity to implement more complex constructs (e.g. more state-full
tracing).
This patch extends the tail-call mechanism to allow a BPF program of
one type to call a BPF program of another type. E.g. a kprobe BPF
program (working with a struct pt_regs context) can call a BPF program
with a more extensive context. The BPF program type is being extended
to provide can_cast() and cast_context() callback functions to handle
the context conversion.
The program array holding BPF programs that you can tail-call into
continues to require that all programs are of the same type. But when
a compatibility check is made in a program that performs a tail-call,
the can_cast() function is called (if available) to allow the target
type to determine whether it can handle the conversion of a context
from the source type to the target type. If can_cast() is not provided
by the program type, casting is denied.
During execution, the cast_context() function is called (if available)
to perform the conversion of the current context to the context that the
target type expects. Since the program type of the executing BPF program
is not explicitly known during execution, the verifier inserts an
instruction right before the tail-call to assign the current BPF program
type to R4.
The interpreter calls cast_context() using the program type in R4 as
source program type, the program type associated with the program array
as target program type, and the context as provided in R1.
The bpf_prog_types array is now being exposed to the rest of the BPF
code (where before it was local to just the syscall handling) because
the can_cast and cat_context operations need to be accessible.
There is no noticeable effect on BPF program types that do not implement
this new feature.
A JIT implementation is not available yet in this first iteration.
Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
---
include/linux/bpf.h | 4 ++++
kernel/bpf/core.c | 27 ++++++++++++++++++++++++++-
kernel/bpf/syscall.c | 2 +-
kernel/bpf/verifier.c | 16 ++++++++++++----
4 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de18227b3d95..117d2bae51b9 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -281,6 +281,9 @@ bpf_ctx_record_field_size(struct bpf_insn_access_aux *aux, u32 size)
struct bpf_prog_ops {
int (*test_run)(struct bpf_prog *prog, const union bpf_attr *kattr,
union bpf_attr __user *uattr);
+ bool (*can_cast)(enum bpf_prog_type stype, enum bpf_prog_type ttype);
+ void *(*cast_context)(enum bpf_prog_type stype,
+ enum bpf_prog_type ttype, void *ctx);
};
struct bpf_verifier_ops {
@@ -528,6 +531,7 @@ extern const struct file_operations bpf_prog_fops;
#undef BPF_PROG_TYPE
#undef BPF_MAP_TYPE
+extern const struct bpf_prog_ops * const bpf_prog_types[];
extern const struct bpf_prog_ops bpf_offload_prog_ops;
extern const struct bpf_verifier_ops tc_cls_act_analyzer_ops;
extern const struct bpf_verifier_ops xdp_analyzer_ops;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ef88b167959d..1b7c718d4e9d 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1426,10 +1426,12 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
CONT;
JMP_TAIL_CALL: {
+ void *ctx = (void *) (unsigned long) BPF_R1;
struct bpf_map *map = (struct bpf_map *) (unsigned long) BPF_R2;
struct bpf_array *array = container_of(map, struct bpf_array, map);
struct bpf_prog *prog;
u32 index = BPF_R3;
+ u32 type = BPF_R4;
if (unlikely(index >= array->map.max_entries))
goto out;
@@ -1441,6 +1443,14 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
prog = READ_ONCE(array->ptrs[index]);
if (!prog)
goto out;
+ if (prog->aux->ops->cast_context) {
+ ctx = prog->aux->ops->cast_context(type, prog->type,
+ ctx);
+ if (!ctx)
+ goto out;
+
+ BPF_R1 = (u64) ctx;
+ }
/* ARG1 at this point is guaranteed to point to CTX from
* the verifier side due to the fact that the tail call is
@@ -1637,6 +1647,20 @@ bool bpf_prog_array_compatible(struct bpf_array *array,
array->owner_jited == fp->jited;
}
+bool bpf_prog_array_can_cast(struct bpf_array *array, const struct bpf_prog *fp)
+{
+ const struct bpf_prog_ops *ops;
+
+ if (array->owner_jited != fp->jited)
+ return false;
+
+ ops = bpf_prog_types[array->owner_prog_type];
+ if (ops->can_cast)
+ return ops->can_cast(fp->type, array->owner_prog_type);
+
+ return false;
+}
+
static int bpf_check_tail_call(const struct bpf_prog *fp)
{
struct bpf_prog_aux *aux = fp->aux;
@@ -1650,7 +1674,8 @@ static int bpf_check_tail_call(const struct bpf_prog *fp)
continue;
array = container_of(map, struct bpf_array, map);
- if (!bpf_prog_array_compatible(array, fp))
+ if (!bpf_prog_array_compatible(array, fp) &&
+ !bpf_prog_array_can_cast(array, fp))
return -EINVAL;
}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ec7c552af76b..d558d979100f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1110,7 +1110,7 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
return err;
}
-static const struct bpf_prog_ops * const bpf_prog_types[] = {
+const struct bpf_prog_ops * const bpf_prog_types[] = {
#define BPF_PROG_TYPE(_id, _name) \
[_id] = & _name ## _prog_ops,
#define BPF_MAP_TYPE(_id, _ops)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1b9496c41383..b49820e82cf8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7629,9 +7629,10 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
insn->imm = 0;
insn->code = BPF_JMP | BPF_TAIL_CALL;
+ cnt = 0;
aux = &env->insn_aux_data[i + delta];
if (!bpf_map_ptr_unpriv(aux))
- continue;
+ goto privileged;
/* instead of changing every JIT dealing with tail_call
* emit two extra insns:
@@ -7646,13 +7647,20 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
map_ptr = BPF_MAP_PTR(aux->map_state);
insn_buf[0] = BPF_JMP_IMM(BPF_JGE, BPF_REG_3,
- map_ptr->max_entries, 2);
+ map_ptr->max_entries, 3);
insn_buf[1] = BPF_ALU32_IMM(BPF_AND, BPF_REG_3,
container_of(map_ptr,
struct bpf_array,
map)->index_mask);
- insn_buf[2] = *insn;
- cnt = 3;
+ cnt = 2;
+
+privileged:
+ /* store the BPF program type of the currnet program in
+ * R4 so it is known in case this tail call requires
+ * casting the context to a different program type
+ */
+ insn_buf[cnt++] = BPF_MOV64_IMM(BPF_REG_4, prog->type);
+ insn_buf[cnt++] = *insn;
new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
--
2.20.1
^ permalink raw reply related
* [PATCH 2/2] bpf: initial implementation for BPF_PROG_TYPE_GTRACE
From: Kris Van Hees @ 2019-02-25 15:54 UTC (permalink / raw)
To: netdev
Many tracing use-cases associate events with the task that triggers
them. BPF programs triggered from kernel probes/events do not provide
information about the current task. Some information can be obtained
with helper functions but it is limited and adding more helper functions
doesn't seem like a good idea in the long term.
This patch implements a new BPF program type (gtrace) to offer support
for generic tracing BPF programs. It is meant to be used through the
mechanism for converting contexts when performing a tail call from one
program type to another. Programs of type BPF_PROG_TYPE_GTRACE are not
triggered from anywhere explicitly. Adding this higher level program
type is a better fit for programs that are not specific to one probe or
event type, but rather operate on a variety of them, as tracers often
do.
The context for BPF_PROG_TYPE_GTRACE programs is in this initial
version a combination of struct pt_regs content and select fields from
the struct task_struct. As can be seen in the included sample code,
this makes it possible to no longer depend on helper functions to get
to task data.
Only one context will ever be in use at a time on a given CPU, because
the context is only initialised when the program is called through a
tail-call from a different (supported) program type.
Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
---
include/linux/bpf_gtrace.h | 27 +++++
include/linux/bpf_types.h | 3 +
include/uapi/linux/bpf.h | 1 +
kernel/trace/Kconfig | 7 ++
kernel/trace/Makefile | 1 +
kernel/trace/gtrace.c | 205 +++++++++++++++++++++++++++++++++
samples/bpf/Makefile | 3 +
samples/bpf/bpf_load.c | 10 +-
samples/bpf/tracex5b_kern.c | 71 ++++++++++++
samples/bpf/tracex5b_user.c | 52 +++++++++
tools/include/uapi/linux/bpf.h | 1 +
tools/lib/bpf/libbpf.c | 1 +
tools/lib/bpf/libbpf_probes.c | 1 +
13 files changed, 380 insertions(+), 3 deletions(-)
create mode 100644 include/linux/bpf_gtrace.h
create mode 100644 kernel/trace/gtrace.c
create mode 100644 samples/bpf/tracex5b_kern.c
create mode 100644 samples/bpf/tracex5b_user.c
diff --git a/include/linux/bpf_gtrace.h b/include/linux/bpf_gtrace.h
new file mode 100644
index 000000000000..10c71e27d714
--- /dev/null
+++ b/include/linux/bpf_gtrace.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ */
+#ifndef _UAPI__LINUX_BPF_GTRACE_H__
+#define _UAPI__LINUX_BPF_GTRACE_H__
+
+#include <asm/bpf_perf_event.h>
+
+struct bpf_gtrace_context {
+ /* CPU registers */
+ bpf_user_pt_regs_t regs;
+
+ /* Current task info */
+ u64 task; /* current */
+ u64 state; /* current->state */
+ u32 prio; /* current->prio */
+ u32 cpu; /* current->cpu or current->thread_info->cpu */
+ u32 tid; /* current->pid */
+ u32 pid; /* current->tgid */
+ u32 ppid; /* current->real_parent->tgid */
+ u32 uid; /* from_kuid(&init_user_ns, current_real_cred()->uid */
+ u32 gid; /* from_kgid(&init_user_ns, current_real_cred()->gid */
+ u32 euid; /* from_kuid(&init_user_ns, current_real_cred()->euid */
+ u32 egid; /* from_kgid(&init_user_ns, current_real_cred()->egid */
+};
+
+#endif /* _UAPI__LINUX_BPF_GTRACE_H__ */
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 08bf2f1fe553..4a2961db6cdd 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -35,6 +35,9 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LIRC_MODE2, lirc_mode2)
#ifdef CONFIG_INET
BPF_PROG_TYPE(BPF_PROG_TYPE_SK_REUSEPORT, sk_reuseport)
#endif
+#ifdef CONFIG_BPF_GTRACE
+BPF_PROG_TYPE(BPF_PROG_TYPE_GTRACE, gtrace)
+#endif
BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..6b463071b245 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -166,6 +166,7 @@ enum bpf_prog_type {
BPF_PROG_TYPE_LIRC_MODE2,
BPF_PROG_TYPE_SK_REUSEPORT,
BPF_PROG_TYPE_FLOW_DISSECTOR,
+ BPF_PROG_TYPE_GTRACE,
};
enum bpf_attach_type {
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index fa8b1fe824f3..8beed291ee68 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -792,6 +792,13 @@ config GCOV_PROFILE_FTRACE
Note that on a kernel compiled with this config, ftrace will
run significantly slower.
+config BPF_GTRACE
+ bool "Generic BPF Tracing Support"
+ depends on BPF_SYSCALL
+ default y
+ help
+ Enable generic tracing BPF program support.
+
endif # FTRACE
endif # TRACING_SUPPORT
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index c2b2148bb1d2..4deeff2a8b70 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -82,6 +82,7 @@ endif
obj-$(CONFIG_DYNAMIC_EVENTS) += trace_dynevent.o
obj-$(CONFIG_PROBE_EVENTS) += trace_probe.o
obj-$(CONFIG_UPROBE_EVENTS) += trace_uprobe.o
+obj-$(CONFIG_BPF_GTRACE) += gtrace.o
obj-$(CONFIG_TRACEPOINT_BENCHMARK) += trace_benchmark.o
diff --git a/kernel/trace/gtrace.c b/kernel/trace/gtrace.c
new file mode 100644
index 000000000000..5ea2d4d7d0c4
--- /dev/null
+++ b/kernel/trace/gtrace.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ */
+#include <linux/bpf.h>
+#include <linux/bpf_gtrace.h>
+#include <linux/filter.h>
+#include <linux/ptrace.h>
+#include <linux/sched.h>
+
+struct bpf_gtrace_ctx {
+ struct pt_regs *regs;
+ struct task_struct *task;
+};
+
+static const struct bpf_func_proto *
+gtrace_get_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+ switch (func_id) {
+ case BPF_FUNC_trace_printk:
+ return bpf_get_trace_printk_proto();
+ default:
+ return NULL;
+ }
+}
+
+static bool gtrace_is_valid_access(int off, int size, enum bpf_access_type type,
+ const struct bpf_prog *prog,
+ struct bpf_insn_access_aux *info)
+{
+ /* Ensure offset is within the context structure. */
+ if (off < 0 || off >= sizeof(struct bpf_gtrace_context))
+ return false;
+
+ /* Only READ access is allowed. */
+ if (type != BPF_READ)
+ return false;
+
+ /* Ensure offset is aligned (verifier guarantees size > 0). */
+ if (off % size != 0)
+ return false;
+
+ switch (off) {
+ case bpf_ctx_range(struct bpf_gtrace_context, task):
+ case bpf_ctx_range(struct bpf_gtrace_context, state):
+ bpf_ctx_record_field_size(info, sizeof(u64));
+ if (bpf_ctx_narrow_access_ok(off, size, sizeof(u64)))
+ return true;
+ break;
+ case bpf_ctx_range(struct bpf_gtrace_context, prio):
+ case bpf_ctx_range(struct bpf_gtrace_context, cpu):
+ case bpf_ctx_range(struct bpf_gtrace_context, tid):
+ case bpf_ctx_range(struct bpf_gtrace_context, pid):
+ case bpf_ctx_range(struct bpf_gtrace_context, ppid):
+ case bpf_ctx_range(struct bpf_gtrace_context, uid):
+ case bpf_ctx_range(struct bpf_gtrace_context, gid):
+ case bpf_ctx_range(struct bpf_gtrace_context, euid):
+ case bpf_ctx_range(struct bpf_gtrace_context, egid):
+ bpf_ctx_record_field_size(info, sizeof(u32));
+ if (bpf_ctx_narrow_access_ok(off, size, sizeof(u32)))
+ return true;
+ break;
+ default:
+ if (size == sizeof(unsigned long))
+ return true;
+ }
+
+ return false;
+}
+
+#define BPF_LDX_CTX_PTR(type, member, si) \
+ BPF_LDX_MEM(BPF_FIELD_SIZEOF(type, member), \
+ (si)->dst_reg, (si)->src_reg, offsetof(type, member))
+#define BPF_LDX_LNK_PTR(type, member, si) \
+ BPF_LDX_MEM(BPF_FIELD_SIZEOF(type, member), \
+ (si)->dst_reg, (si)->dst_reg, offsetof(type, member))
+#define BPF_LDX_CTX_FIELD(type, member, si, target_size) \
+ BPF_LDX_MEM(BPF_FIELD_SIZEOF(type, member), \
+ (si)->dst_reg, (si)->src_reg, \
+ ({ \
+ *(target_size) = FIELD_SIZEOF(type, member); \
+ offsetof(type, member); \
+ }))
+#define BPF_LDX_LNK_FIELD(type, member, si, target_size) \
+ BPF_LDX_MEM(BPF_FIELD_SIZEOF(type, member), \
+ (si)->dst_reg, (si)->dst_reg, \
+ ({ \
+ *(target_size) = FIELD_SIZEOF(type, member); \
+ offsetof(type, member); \
+ }))
+
+static u32 gtrace_convert_ctx_access(enum bpf_access_type type,
+ const struct bpf_insn *si,
+ struct bpf_insn *insn_buf,
+ struct bpf_prog *prog, u32 *target_size)
+{
+ struct bpf_insn *insn = insn_buf;
+
+ switch (si->off) {
+ case offsetof(struct bpf_gtrace_context, task):
+ *insn++ = BPF_LDX_CTX_FIELD(struct bpf_gtrace_ctx, task, si,
+ target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, state):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct task_struct, state, si,
+ target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, prio):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct task_struct, prio, si,
+ target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, cpu):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ *insn++ = BPF_LDX_LNK_FIELD(struct task_struct, cpu, si,
+ target_size);
+#else
+ *insn++ = BPF_LDX_LNK_PTR(struct task_struct, stack, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct thread_info, cpu, si,
+ target_size);
+#endif
+ break;
+ case offsetof(struct bpf_gtrace_context, tid):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct task_struct, pid, si,
+ target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, pid):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct task_struct, tgid, si,
+ target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, ppid):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_PTR(struct task_struct, real_parent, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct task_struct, tgid, si,
+ target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, uid):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_PTR(struct task_struct, cred, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct cred, uid, si, target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, gid):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_PTR(struct task_struct, cred, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct cred, gid, si, target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, euid):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_PTR(struct task_struct, cred, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct cred, euid, si, target_size);
+ break;
+ case offsetof(struct bpf_gtrace_context, egid):
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, task, si);
+ *insn++ = BPF_LDX_LNK_PTR(struct task_struct, cred, si);
+ *insn++ = BPF_LDX_LNK_FIELD(struct cred, egid, si, target_size);
+ break;
+ default:
+ *insn++ = BPF_LDX_CTX_PTR(struct bpf_gtrace_ctx, regs, si);
+ *insn++ = BPF_LDX_MEM(BPF_SIZEOF(long), si->dst_reg, si->dst_reg,
+ si->off);
+ break;
+ }
+
+ return insn - insn_buf;
+}
+
+const struct bpf_verifier_ops gtrace_verifier_ops = {
+ .get_func_proto = gtrace_get_func_proto,
+ .is_valid_access = gtrace_is_valid_access,
+ .convert_ctx_access = gtrace_convert_ctx_access,
+};
+
+static bool gtrace_can_cast(enum bpf_prog_type stype, enum bpf_prog_type ttype)
+{
+ if (stype == BPF_PROG_TYPE_KPROBE)
+ return true;
+
+ return false;
+}
+
+DEFINE_PER_CPU(struct bpf_gtrace_ctx, gtrace_ctx);
+
+static void *gtrace_cast_context(enum bpf_prog_type stype,
+ enum bpf_prog_type ttype, void *ctx)
+{
+ struct bpf_gtrace_ctx *gctx;
+
+ if (stype == BPF_PROG_TYPE_KPROBE) {
+ gctx = this_cpu_ptr(>race_ctx);
+ gctx->regs = (struct pt_regs *)ctx;
+ gctx->task = current;
+
+ return gctx;
+ }
+
+ return NULL;
+}
+
+const struct bpf_prog_ops gtrace_prog_ops = {
+ .can_cast = gtrace_can_cast,
+ .cast_context = gtrace_cast_context,
+};
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index a0ef7eddd0b3..a5f8988acdb2 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -15,6 +15,7 @@ hostprogs-y += tracex2
hostprogs-y += tracex3
hostprogs-y += tracex4
hostprogs-y += tracex5
+hostprogs-y += tracex5b
hostprogs-y += tracex6
hostprogs-y += tracex7
hostprogs-y += test_probe_write_user
@@ -69,6 +70,7 @@ tracex2-objs := bpf_load.o tracex2_user.o
tracex3-objs := bpf_load.o tracex3_user.o
tracex4-objs := bpf_load.o tracex4_user.o
tracex5-objs := bpf_load.o tracex5_user.o
+tracex5b-objs := bpf_load.o tracex5b_user.o
tracex6-objs := bpf_load.o tracex6_user.o
tracex7-objs := bpf_load.o tracex7_user.o
load_sock_ops-objs := bpf_load.o load_sock_ops.o
@@ -120,6 +122,7 @@ always += tracex2_kern.o
always += tracex3_kern.o
always += tracex4_kern.o
always += tracex5_kern.o
+always += tracex5b_kern.o
always += tracex6_kern.o
always += tracex7_kern.o
always += sock_flags_kern.o
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index eae7b635343d..847b46a64315 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -87,6 +87,7 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
bool is_sockops = strncmp(event, "sockops", 7) == 0;
bool is_sk_skb = strncmp(event, "sk_skb", 6) == 0;
bool is_sk_msg = strncmp(event, "sk_msg", 6) == 0;
+ bool is_gtrace = strncmp(event, "gtrace", 6) == 0;
size_t insns_cnt = size / sizeof(struct bpf_insn);
enum bpf_prog_type prog_type;
char buf[256];
@@ -120,6 +121,8 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
prog_type = BPF_PROG_TYPE_SK_SKB;
} else if (is_sk_msg) {
prog_type = BPF_PROG_TYPE_SK_MSG;
+ } else if (is_gtrace) {
+ prog_type = BPF_PROG_TYPE_GTRACE;
} else {
printf("Unknown event '%s'\n", event);
return -1;
@@ -140,8 +143,8 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
if (is_xdp || is_perf_event || is_cgroup_skb || is_cgroup_sk)
return 0;
- if (is_socket || is_sockops || is_sk_skb || is_sk_msg) {
- if (is_socket)
+ if (is_socket || is_sockops || is_sk_skb || is_sk_msg || is_gtrace) {
+ if (is_socket || is_gtrace)
event += 6;
else
event += 7;
@@ -643,7 +646,8 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
memcmp(shname, "cgroup/", 7) == 0 ||
memcmp(shname, "sockops", 7) == 0 ||
memcmp(shname, "sk_skb", 6) == 0 ||
- memcmp(shname, "sk_msg", 6) == 0) {
+ memcmp(shname, "sk_msg", 6) == 0 ||
+ memcmp(shname, "gtrace", 6) == 0) {
ret = load_and_attach(shname, data->d_buf,
data->d_size);
if (ret != 0)
diff --git a/samples/bpf/tracex5b_kern.c b/samples/bpf/tracex5b_kern.c
new file mode 100644
index 000000000000..3bcc4779df3f
--- /dev/null
+++ b/samples/bpf/tracex5b_kern.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ */
+#include <linux/ptrace.h>
+#include <linux/version.h>
+#include <uapi/linux/bpf.h>
+#include <linux/bpf_gtrace.h>
+#include <uapi/linux/seccomp.h>
+#include <uapi/linux/unistd.h>
+#include "syscall_nrs.h"
+#include "bpf_helpers.h"
+
+#define PROG(F) SEC("gtrace/"__stringify(F)) int bpf_func_##F
+
+struct bpf_map_def SEC("maps") progs = {
+ .type = BPF_MAP_TYPE_PROG_ARRAY,
+ .key_size = sizeof(u32),
+ .value_size = sizeof(u32),
+#ifdef __mips__
+ .max_entries = 6000, /* MIPS n64 syscalls start at 5000 */
+#else
+ .max_entries = 1024,
+#endif
+};
+
+/* we jump here when syscall number == __NR_write */
+PROG(SYS__NR_write)(struct bpf_gtrace_context *ctx)
+{
+ char fmt1[] = "write for task %p (state %x, ppid %d)\n";
+ char fmt2[] = "write for task %p (tid %d, pid %d)\n";
+ char fmt3[] = "write for task %p (uid %d, gid %d)\n";
+ char fmt4[] = "write for task %p (euid %d, egid %d)\n";
+ bpf_trace_printk(fmt1, sizeof(fmt1), ctx->task, ctx->state, ctx->ppid);
+ bpf_trace_printk(fmt2, sizeof(fmt2), ctx->task, ctx->tid, ctx->pid);
+ bpf_trace_printk(fmt3, sizeof(fmt3), ctx->task, ctx->uid, ctx->gid);
+ bpf_trace_printk(fmt4, sizeof(fmt4), ctx->task, ctx->euid, ctx->egid);
+ return 0;
+}
+
+PROG(SYS__NR_read)(struct bpf_gtrace_context *ctx)
+{
+ char fmt[] = "read for task %p\n";
+ bpf_trace_printk(fmt, sizeof(fmt), ctx->task);
+ return 0;
+}
+
+PROG(SYS__NR_mmap)(struct bpf_gtrace_context *ctx)
+{
+ char fmt[] = "mmap for task %p\n";
+ bpf_trace_printk(fmt, sizeof(fmt), ctx->task);
+ return 0;
+}
+
+SEC("kprobe/__seccomp_filter")
+int bpf_prog1(struct pt_regs *ctx)
+{
+ int sc_nr = (int)PT_REGS_PARM1(ctx);
+
+ /* dispatch into next BPF program depending on syscall number */
+ bpf_tail_call(ctx, &progs, sc_nr);
+
+ /* fall through -> unknown syscall */
+ if (sc_nr >= __NR_getuid && sc_nr <= __NR_getsid) {
+ char fmt[] = "syscall=%d (one of get/set uid/pid/gid)\n";
+ bpf_trace_printk(fmt, sizeof(fmt), sc_nr);
+ }
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+u32 _version SEC("version") = LINUX_VERSION_CODE;
diff --git a/samples/bpf/tracex5b_user.c b/samples/bpf/tracex5b_user.c
new file mode 100644
index 000000000000..f59d75e7805b
--- /dev/null
+++ b/samples/bpf/tracex5b_user.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ */
+#include <stdio.h>
+#include <linux/bpf.h>
+#include <unistd.h>
+#include <linux/filter.h>
+#include <linux/seccomp.h>
+#include <sys/prctl.h>
+#include <bpf/bpf.h>
+#include "bpf_load.h"
+#include <sys/resource.h>
+
+/* install fake seccomp program to enable seccomp code path inside the kernel,
+ * so that our kprobe attached to seccomp_phase1() can be triggered
+ */
+static void install_accept_all_seccomp(void)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+ struct sock_fprog prog = {
+ .len = (unsigned short)(sizeof(filter)/sizeof(filter[0])),
+ .filter = filter,
+ };
+ if (prctl(PR_SET_SECCOMP, 2, &prog))
+ perror("prctl");
+}
+
+int main(int ac, char **argv)
+{
+ FILE *f;
+ char filename[256];
+ struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+
+ snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+ setrlimit(RLIMIT_MEMLOCK, &r);
+
+ if (load_bpf_file(filename)) {
+ printf("%s", bpf_log_buf);
+ return 1;
+ }
+
+ install_accept_all_seccomp();
+
+ f = popen("dd if=/dev/zero of=/dev/null count=5", "r");
+ (void) f;
+
+ read_trace_pipe();
+
+ return 0;
+}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..6b463071b245 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -166,6 +166,7 @@ enum bpf_prog_type {
BPF_PROG_TYPE_LIRC_MODE2,
BPF_PROG_TYPE_SK_REUSEPORT,
BPF_PROG_TYPE_FLOW_DISSECTOR,
+ BPF_PROG_TYPE_GTRACE,
};
enum bpf_attach_type {
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b38dcbe7460a..297a0a83086e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1682,6 +1682,7 @@ static bool bpf_prog_type__needs_kver(enum bpf_prog_type type)
case BPF_PROG_TYPE_PERF_EVENT:
return false;
case BPF_PROG_TYPE_KPROBE:
+ case BPF_PROG_TYPE_GTRACE:
default:
return true;
}
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index 8c3a1c04dcb2..2b5bfcda7606 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -97,6 +97,7 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
case BPF_PROG_TYPE_LIRC_MODE2:
case BPF_PROG_TYPE_SK_REUSEPORT:
case BPF_PROG_TYPE_FLOW_DISSECTOR:
+ case BPF_PROG_TYPE_GTRACE:
default:
break;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Stephen Hemminger @ 2019-02-25 16:01 UTC (permalink / raw)
To: Sheng Lan
Cc: davem, netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji,
liuzhiqiang26
In-Reply-To: <c610177c-60d5-ef96-e6b7-23f3e49245a2@huawei.com>
On Mon, 25 Feb 2019 22:49:39 +0800
Sheng Lan <lansheng@huawei.com> wrote:
> From: Sheng Lan <lansheng@huawei.com>
> Subject: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
>
> It can be reproduced by following steps:
> 1. virtio_net NIC is configured with gso/tso on
> 2. configure nginx as http server with an index file bigger than 1M bytes
> 3. use tc netem to produce duplicate packets and delay:
> tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
> 4. continually curl the nginx http server to get index file on client
> 5. BUG_ON is seen quickly
>
> [10258690.371129] kernel BUG at net/core/skbuff.c:4028!
> [10258690.371748] invalid opcode: 0000 [#1] SMP PTI
> [10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
> [10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
> [10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
> [10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
> [10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
> [10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
> [10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
> [10258690.372094] Call Trace:
> [10258690.372094] <IRQ>
> [10258690.372094] skb_to_sgvec+0x11/0x40
> [10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
> [10258690.372094] dev_hard_start_xmit+0x9b/0x200
> [10258690.372094] sch_direct_xmit+0xff/0x260
> [10258690.372094] __qdisc_run+0x15e/0x4e0
> [10258690.372094] net_tx_action+0x137/0x210
> [10258690.372094] __do_softirq+0xd6/0x2a9
> [10258690.372094] irq_exit+0xde/0xf0
> [10258690.372094] smp_apic_timer_interrupt+0x74/0x140
> [10258690.372094] apic_timer_interrupt+0xf/0x20
> [10258690.372094] </IRQ>
>
> In __skb_to_sgvec, the skb->len is not equal to the sum of the skb's
> linear data size and nonlinear data size, thus BUG_ON triggered. The
> bad skb's nonlinear data size is less than skb->data_len, because the
> skb is cloned and a part of related cloned skb's nonlinear data is
> split off.
>
> Duplicate packet is cloned by skb_clone in netem_enqueue and may be delayed
> some time in qdisc. Due to the delay time, the original skb will be pushed
> again later in __tcp_push_pending_frames when tcp receives new packets.
> In tcp_write_xmit, when the tcp_mss_split_point returns a smaller limit,
> the original skb will be fragmented and the skb's nonlinear data will be
> split off. The length of the skb cloned by netem will not be updated.
> When we use virtio_net NIC, the duplicated cloned skb will be filled into
> a scatter-gather list in __skb_to_sgvec and trigger the BUG_ON.
>
> Here I replace the skb_clone with skb_copy in netem_enqueue to ensure
> the duplicated skb's nonlinear data is independent.
>
> Signed-off-by: Sheng Lan <lansheng@huawei.com>
> Reported-by: Qin Ji <jiqin.ji@huawei.com>
>
> Fixes: 0afb51e7 ("netem: reinsert for duplication")
This sounds like a bug in the other layers (either TCP or Virtio net)
not handling a cloned skb properly.
^ permalink raw reply
* Re: [PATCH net-next 01/12] net: sched: flower: don't check for rtnl on head dereference
From: Vlad Buslov @ 2019-02-25 16:11 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpVHggO6vQDr6RaWdr-g1nZGScO_mkWNVqaDXQSVbdiOCA@mail.gmail.com>
On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Feb 21, 2019 at 9:45 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>>
>> On Wed 20 Feb 2019 at 22:33, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Tue, Feb 19, 2019 at 1:46 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >>
>> >>
>> >> On Mon 18 Feb 2019 at 19:08, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >> > On Wed, Feb 13, 2019 at 11:47 PM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >> >>
>> >> >> Flower classifier only changes root pointer during init and destroy. Cls
>> >> >> API implements reference counting for tcf_proto, so there is no danger of
>> >> >> concurrent access to tp when it is being destroyed, even without protection
>> >> >> provided by rtnl lock.
>> >> >
>> >> > How about atomicity? Refcnt doesn't guarantee atomicity, how do
>> >> > you make sure two concurrent modifications are atomic?
>> >>
>> >> In order to guarantee atomicity I lock shared flower classifier data
>> >> structures with tp->lock in following patches.
>> >
>> > Sure, I meant the atomicity of the _whole_ change, as you know
>> > the TC filters are stored in hierarchical structures: a block, a chain,
>> > a tp struct, some type-specific data structure like a hash table.
>> >
>> > Locking tp only solves a partial of the atomicity here. Are you
>> > going to restart the whole change from top down to the bottom?
>>
>> You mean in cases where there is a conflict? Yes, processing EAGAIN in
>> tc_new_tfilter() involves releasing and re-obtaining all locks and
>> references.
>
> Sure, restart only happens when a conflict is detected, this is
> why I called it worst scenario.
>
>
>>
>> >
>> >
>> >>
>> >> >
>> >> >
>> >> >>
>> >> >> Implement new function fl_head_dereference() to dereference tp->root
>> >> >> without checking for rtnl lock. Use it in all flower function that obtain
>> >> >> head pointer instead of rtnl_dereference().
>> >> >>
>> >> >
>> >> > So what lock protects RCU writers after this patch?
>> >>
>> >> I explained it in comment for fl_head_dereference(), but should have
>> >> copied this information to changelog as well:
>> >> Flower classifier only changes root pointer during init and destroy.
>> >> Cls API implements reference counting for tcf_proto, so there is no
>> >> danger of concurrent access to tp when it is being destroyed, even
>> >> without protection provided by rtnl lock.
>> >
>> > So you are saying an RCU pointer is okay to deference without
>> > any lock eve without RCU read lock, right?
>> >
>> >
>> >>
>> >> In initial version of this change I used tp->lock to protect tp->root
>> >> access and verified it with lockdep, but during internal review Jiri
>> >> noted that this is not needed in current flower implementation.
>> >
>> > Let's see what you have on top of your own branch
>> > unlocked_flower_cong_1:
>> >
>> > 1458 static int fl_change(struct net *net, struct sk_buff *in_skb,
>> > 1459 struct tcf_proto *tp, unsigned long base,
>> > 1460 u32 handle, struct nlattr **tca,
>> > 1461 void **arg, bool ovr, bool rtnl_held,
>> > 1462 struct netlink_ext_ack *extack)
>> > 1463 {
>> > 1464 struct cls_fl_head *head = fl_head_dereference(tp);
>> >
>> > At the point of line 1464, there is no lock taken, tp->lock is taken
>> > after it, block->lock or chain lock is already unlocked before ->change().
>> >
>> > So, what protects this RCU structure? According to RCU, it must be
>> > either RCU read lock or some writer lock. I see none here. :(
>> >
>> > What am I missing?
>>
>> Initially I had flower implementation that protected tp->root access
>> with tp->lock, re-obtaining lock and head reference each time it was
>> used, sometimes multiple times per function (head reference is usually
>> obtained in beginning of every flower API function and used multiple
>> times afterwards). This complicated the code and reduced parallelism.
>> However, in case of flower classifier tp->root is never reassigned after
>> init, so essentially there is no "CU" part in this "RCU" pointer. This
>> realization allowed me to refactor unlocked flower code to look much
>> nicer and perform better. Am I missing something in flower classifier
>> implementation?
>
> So if it is no longer RCU any more, why do you still use
> rcu_dereference_protected()? That is, why not just deref it as a raw
> pointer?
>
> And, I don't think I can buy your argument here. The RCU infrastructure
> should not be changed even after your patches, the fast path is still
> protocted by RCU read lock, while the slow path now is protected by
> some smaller-scope locks. What makes cls_flower so unique that
> it doesn't even need RCU here? tp->root is not reassigned but it is still
> freed via RCU infra, that is in fl_destroy_sleepable().
>
> Thanks.
My cls API patch set introduced reference counting for tcf_proto
structure. With that change tp->ops->destroy() (which calls fl_destroy()
and fl_destroy_sleepable(), in case of flower classifier) is only called
after last reference to tp is released. All slow path users of tp->ops
must obtain reference to tp, so concurrent call to fl_destroy() is not
possible. Before this change tcf_proto structure didn't have reference
counting support and required users to obtain rtnl mutex before calling
its ops callbacks. This was verified in flower by using rtnl_dereference
to obtain tp->root.
^ permalink raw reply
* [PATCH net-next 2/2] net: phy: marvell10g: Use the generic C45 helper to read the 2110 features
From: Maxime Chevallier @ 2019-02-25 16:14 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal
In-Reply-To: <20190225161407.19510-1-maxime.chevallier@bootlin.com>
Contrary to the 3310, the 2110 PHY correctly reports it's 2.5G/5G
abilities. We can therefore use the genphy_c45_pma_read_abilities helper
to build the list of features.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/marvell10g.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index ab71da55c1e0..79106e70010f 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -472,7 +472,7 @@ static struct phy_driver mv3310_drivers[] = {
.phy_id = MARVELL_PHY_ID_88E2110,
.phy_id_mask = MARVELL_PHY_ID_MASK,
.name = "mv88x2110",
- .features = PHY_10GBIT_FEATURES,
+ .get_features = genphy_c45_pma_read_abilities,
.probe = mv3310_probe,
.soft_reset = gen10g_no_soft_reset,
.config_init = mv3310_config_init,
--
2.19.2
^ permalink raw reply related
* [PATCH net-next 1/2] net: phy: marvell10g: Let genphy_c45_pma_read_abilities set Aneg bit
From: Maxime Chevallier @ 2019-02-25 16:14 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal
In-Reply-To: <20190225161407.19510-1-maxime.chevallier@bootlin.com>
The genphy_c45_pma_read_abilities helper now sets the Autoneg ability
in phydev->supported according to what the AN MMD reports.
We therefore don't need to manually do that in mv3310_get_features().
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/marvell10g.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 580e91deadbc..ab71da55c1e0 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -268,16 +268,6 @@ static int mv3310_get_features(struct phy_device *phydev)
{
int ret, val;
- if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
- val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
- if (val < 0)
- return val;
-
- if (val & MDIO_AN_STAT1_ABLE)
- linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
- phydev->supported);
- }
-
ret = genphy_c45_pma_read_abilities(phydev);
if (ret)
return ret;
--
2.19.2
^ permalink raw reply related
* [PATCH net-next 0/2] net: phy: marvell10g: Clean .get_features by using C45 helpers
From: Maxime Chevallier @ 2019-02-25 16:14 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King, linux-arm-kernel,
Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal
Recent work on C45 helpers by Heiner made the
genphy_c45_pma_read_abilities function generic enough to use as a
default .get_featutes implementation.
This series removes the remaining redundant code in
mv3310_get_features(), and makes the 2110 PHY use
genphy_c45_pma_read_abilities() directly, since it doesn't have the
issue with the wrong abilities being reported.
Maxime Chevallier (2):
net: phy: marvell10g: Let genphy_c45_pma_read_abilities set Aneg bit
net: phy: marvell10g: Use the generic C45 helper to read the 2110
features
drivers/net/phy/marvell10g.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
--
2.19.2
^ permalink raw reply
* Re: [PATCH net] sctp: get sctphdr by offset in sctp_compute_cksum
From: Xin Long @ 2019-02-25 16:15 UTC (permalink / raw)
To: Neil Horman; +Cc: LKML, network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <20190225140720.GC30699@hmswarspite.think-freely.org>
On Mon, Feb 25, 2019 at 10:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Mon, Feb 25, 2019 at 09:20:44PM +0800, Xin Long wrote:
> > On Mon, Feb 25, 2019 at 8:47 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Mon, Feb 25, 2019 at 07:25:37PM +0800, Xin Long wrote:
> > > > sctp_hdr(skb) only works when skb->transport_header is set properly.
> > > >
> > > > But in the path of nf_conntrack_in: sctp_packet() -> sctp_error()
> > > >
> > > > skb->transport_header is not guaranteed to be right value for sctp.
> > > > It will cause to fail to check the checksum for sctp packets.
> > > >
> > > > So fix it by using offset, which is always right in all places.
> > > >
> > > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > > Reported-by: Li Shuang <shuali@redhat.com>
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > > include/net/sctp/checksum.h | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> > > > index 32ee65a..1c6e6c0 100644
> > > > --- a/include/net/sctp/checksum.h
> > > > +++ b/include/net/sctp/checksum.h
> > > > @@ -61,7 +61,7 @@ static inline __wsum sctp_csum_combine(__wsum csum, __wsum csum2,
> > > > static inline __le32 sctp_compute_cksum(const struct sk_buff *skb,
> > > > unsigned int offset)
> > > > {
> > > > - struct sctphdr *sh = sctp_hdr(skb);
> > > > + struct sctphdr *sh = (struct sctphdr *)(skb->data + offset);
> > > > const struct skb_checksum_ops ops = {
> > > > .update = sctp_csum_update,
> > > > .combine = sctp_csum_combine,
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > > Shouldn't you use skb_set_transport_header and skb_transport_header here?
> > you mean:
> > skb_set_transport_header(skb, offset);
> > sh = sctp_hdr(skb);
> > ?
> >
> > There's no place counting on here to set transport_header.
> > It will be a kinda redundant job, yet skb is 'const'.
> >
> I'm not sure what you mean by "theres no place counting here". We have the
> transport header offset, and you're doing the exact same computation that that
> function does. It seems like we should use it in case the underlying
> implementation changes.
1. skb_set_transport_header() and sctp_hdr() are like:
skb->transport_header = skb->data - skb->head;
skb->transport_header += offset
sh = skb->head + skb->transport_header;
2. in this patch:
sh = (struct sctphdr *)(skb->data + offset); only
I think the 2nd one is better.
I feel it's weird to set transport_header here if it's only for
sctp_hdr(skb) in here.
As for "underlying implementation changes", I don't know exactly the case
but there are quite a few places doing things like:
*hdr = (struct *hdr *)(skb->data + hdroff);
I'd think it's safe. no?
>
> I understand what you are saying regarding the use of a const variable there,
> but perhaps thats an argument for removing the const storage classifier. Better
> still, it would be good to figure out why all paths to this function don't
> already set the transport header offset to begin with (addressing your redundant
> comment)
The issue was reported when going to nf_conntrack by br_netfilter's
bridge-nf-call-iptables.
As you can see on nf_conntrack_in() path, even iphdr is got by:
iph = skb_header_pointer(skb, nhoff, sizeof(_iph), &_iph);
It's impossible to set skb->transport_header when we're not sure iphdr
in linearized memory.
^ permalink raw reply
* RE: [PATCH net-next v3 4/4] dt-bindings: net: freescale: enetc: Add connection bindings for ENETC ethernet nodes
From: Claudiu Manoil @ 2019-02-25 16:16 UTC (permalink / raw)
To: Rob Herring
Cc: Shawn Guo, Leo Li, David S . Miller, Alexandru Marginean,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190222233732.GA1518@bogus>
>-----Original Message-----
>From: Rob Herring <robh@kernel.org>
>Sent: Saturday, February 23, 2019 1:38 AM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>Cc: Shawn Guo <shawnguo@kernel.org>; Leo Li <leoyang.li@nxp.com>; David S .
>Miller <davem@davemloft.net>; Alexandru Marginean
><alexandru.marginean@nxp.com>; linux-arm-kernel@lists.infradead.org;
>devicetree@vger.kernel.org; netdev@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH net-next v3 4/4] dt-bindings: net: freescale: enetc: Add
>connection bindings for ENETC ethernet nodes
>
>On Fri, Feb 22, 2019 at 05:04:19PM +0200, Claudiu Manoil wrote:
>> Define connection bindings (external PHY connections and internal
>> links) for the ENETC on-chip ethernet controllers.
>>
>> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
>> ---
>> v3 - added this patch to the set
>>
>> .../devicetree/bindings/net/fsl-enetc.txt | 109 +++++++++++++++++++++
>> 1 file changed, 109 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/net/fsl-enetc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/fsl-enetc.txt
>> b/Documentation/devicetree/bindings/net/fsl-enetc.txt
>> new file mode 100644
>> index 0000000..2fbb998
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/fsl-enetc.txt
>> @@ -0,0 +1,109 @@
>> +* ENETC ethernet nodes - external connection bindings
>> +
>> +The ENETC ethernet controllers are PCIe integrated endpoints (IEPs),
>> +on-chip devices discoverable as standard PCIe endpoints, integrated
>> +into Freescale SoCs. The ENETC devices are self contained, the
>> +information needed for device initialization is available in hardware
>> +(PCIe ECAM area). However, depending on board design, their external
>> +connections are configurable.
>> +As usual for SoCs, device tree nodes may be used to define these
>> +external connections. The rest of the document specifies how
>> +external connections for ENETC ethernet controllers may be defined
>> +via device tree nodes.
>> +
>> +Silicon (SoC) availability (<SoC name>: <SoC DT path/name>)
>> + - LS1028A: [arch/arm64] [...]freescale/fsl-ls1028a.dtsi
>
>This doesn't belong in bindings.
>
>> +
>> +
>> +* ENETC nodes
>> +
>> +Defined in the SoC device tree as "pci" child nodes of the
>> +"pci-host-ecam-generic" compatible "pcie" parent node also known as
>> +the Integrated Endpoint Root Complex (IERC) SoC node.
>
>The host controller attachment is also outside the scope of this binding.
>
>> +
>> +Structure - example (LS1028A):
>> +
>> + pcie@1f0000000 {
>> + compatible = "pci-host-ecam-generic";
>> + device_type = "pci";
>> + ...
>> + enetc_port0: pci@0,0 {
>
>The node name 'pci' is reserved for bridges. This should match the device class if
>possible (ethernet).
>
>> + reg = <0x000000 0 0 0 0>;
>> + };
>> + enetc_port1: pci@0,1 {
>> + reg = <0x000100 0 0 0 0>;
>> + };
>> + ...
>> + }
>> +
>> +Each ENETC node has a device number and a function number (expressed
>> +by its "reg" property and pci node name, i.e. "pci@0,1" represents
>> +device number 0 and functions number 1). Only the standard pci "reg"
>> +property is needed here.
>
>There should be a compatible too.
[...]
Ok to simplifying the text and document strictly the enetc device nodes as
"ethernet" nodes, like "ethernet@<dev_no>,<fcn_no>" (i.e ethernet@0,1).
But what would be the compatible string needed for?
The ethernet device driver doesn't need it, probing is done by the pci system.
Is it ok to use a generic name for the compatible string, like, "fsl,enetc", just
to indicate that the relevant driver is located at ethernet/freescale/enetc/
dir in the source code? (under drivers/net, of course)
Thanks.
Claudiu
^ permalink raw reply
* [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error
From: Nazarov Sergey @ 2019-02-25 16:24 UTC (permalink / raw)
To: David Miller
Cc: paul@paul-moore.com, netdev@vger.kernel.org,
linux-security-module@vger.kernel.org, kuznet@ms2.inr.ac.ru,
yoshfuji@linux-ipv6.org
In-Reply-To: <20190224.173328.1032826011262803545.davem@davemloft.net>
Add __icmp_send function having ip_options struct parameter
Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
---
include/net/icmp.h | 9 ++++++++-
net/ipv4/icmp.c | 7 ++++---
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/include/net/icmp.h b/include/net/icmp.h
index 6ac3a5b..e0f709d 100644
--- a/include/net/icmp.h
+++ b/include/net/icmp.h
@@ -22,6 +22,7 @@
#include <net/inet_sock.h>
#include <net/snmp.h>
+#include <net/ip.h>
struct icmp_err {
int errno;
@@ -39,7 +40,13 @@ struct icmp_err {
struct sk_buff;
struct net;
-void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info);
+void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
+ const struct ip_options *opt);
+static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
+{
+ __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
+}
+
int icmp_rcv(struct sk_buff *skb);
int icmp_err(struct sk_buff *skb, u32 info);
int icmp_init(void);
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 065997f..3f24414 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -570,7 +570,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
* MUST reply to only the first fragment.
*/
-void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
+void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
+ const struct ip_options *opt)
{
struct iphdr *iph;
int room;
@@ -691,7 +692,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
iph->tos;
mark = IP4_REPLY_MARK(net, skb_in->mark);
- if (ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in))
+ if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in, opt))
goto out_unlock;
@@ -742,7 +743,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
local_bh_enable();
out:;
}
-EXPORT_SYMBOL(icmp_send);
+EXPORT_SYMBOL(__icmp_send);
static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
---
^ permalink raw reply related
* [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error
From: Nazarov Sergey @ 2019-02-25 16:27 UTC (permalink / raw)
To: David Miller
Cc: paul@paul-moore.com, netdev@vger.kernel.org,
linux-security-module@vger.kernel.org, kuznet@ms2.inr.ac.ru,
yoshfuji@linux-ipv6.org
In-Reply-To: <20190224.173328.1032826011262803545.davem@davemloft.net>
Extract IP options in cipso_v4_error and use __icmp_send.
Signed-off-by: Sergey Nazarov <s-nazarov@yandex.ru>
---
include/net/ip.h | 2 ++
net/ipv4/cipso_ipv4.c | 17 +++++++++++++++--
net/ipv4/ip_options.c | 22 +++++++++++++++++-----
3 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/include/net/ip.h b/include/net/ip.h
index 8866bfc..f0e8d06 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -667,6 +667,8 @@ static inline int ip_options_echo(struct net *net, struct ip_options *dopt,
}
void ip_options_fragment(struct sk_buff *skb);
+int __ip_options_compile(struct net *net, struct ip_options *opt,
+ struct sk_buff *skb, __be32 *info);
int ip_options_compile(struct net *net, struct ip_options *opt,
struct sk_buff *skb);
int ip_options_get(struct net *net, struct ip_options_rcu **optp,
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 777fa3b..eff86a7 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1735,13 +1735,26 @@ int cipso_v4_validate(const struct sk_buff *skb, unsigned char **option)
*/
void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
{
+ unsigned char optbuf[sizeof(struct ip_options) + 40];
+ struct ip_options *opt = (struct ip_options *)optbuf;
+
if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
return;
+ /*
+ * We might be called above the IP layer,
+ * so we can not use icmp_send and IPCB here.
+ */
+
+ memset(opt, 0, sizeof(struct ip_options));
+ opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
+ if (__ip_options_compile(dev_net(skb->dev), opt, skb, NULL))
+ return;
+
if (gateway)
- icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
+ __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
else
- icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
+ __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
}
/**
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index ed194d4..32a3504 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -251,8 +251,9 @@ static void spec_dst_fill(__be32 *spec_dst, struct sk_buff *skb)
* If opt == NULL, then skb->data should point to IP header.
*/
-int ip_options_compile(struct net *net,
- struct ip_options *opt, struct sk_buff *skb)
+int __ip_options_compile(struct net *net,
+ struct ip_options *opt, struct sk_buff *skb,
+ __be32 *info)
{
__be32 spec_dst = htonl(INADDR_ANY);
unsigned char *pp_ptr = NULL;
@@ -468,11 +469,22 @@ int ip_options_compile(struct net *net,
return 0;
error:
- if (skb) {
- icmp_send(skb, ICMP_PARAMETERPROB, 0, htonl((pp_ptr-iph)<<24));
- }
+ if (info)
+ *info = htonl((pp_ptr-iph)<<24);
return -EINVAL;
}
+
+int ip_options_compile(struct net *net,
+ struct ip_options *opt, struct sk_buff *skb)
+{
+ int ret;
+ __be32 info;
+
+ ret = __ip_options_compile(net, opt, skb, &info);
+ if (ret != 0 && skb)
+ icmp_send(skb, ICMP_PARAMETERPROB, 0, info);
+ return ret;
+}
EXPORT_SYMBOL(ip_options_compile);
/*
---
^ permalink raw reply related
* Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled
From: Eric Dumazet @ 2019-02-25 16:39 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, Stefano Brivio, David S . Miller, David Ahern
In-Reply-To: <20190225081759.GP10051@dhcp-12-139.nay.redhat.com>
On 02/25/2019 12:17 AM, Hangbin Liu wrote:
> On Sun, Feb 24, 2019 at 08:24:51PM -0800, Eric Dumazet wrote:
>>
>>
>> On 02/24/2019 08:12 PM, Hangbin Liu wrote:
>>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
>>> at running time.
>>>
>>
>>
>> Why is it better exactly ?
>>
>> IPv6 can be enabled on the host, but disabled per device
>>
>> /proc/sys/net/ipv6/conf/{name}/disable_ipv6
>
> Sorry, it looks I didn't make it clear in the commit description.
> This issue only occurs when IPv6 is disabled at boot time as there is
> no IPv6 route entry. Disable ipv6 on specific interface is not affected.
> So check ipv6_mod_enabled() is enough and we don't need to worry about
> the rcu_read_lock or the dev status.
>
> Should I update the commit description?
Certainly. Are you telling us skb->dev could be NULL here ?
Because rcu_read_lock() should already be asserted.
^ permalink raw reply
* [PATCH net-next] net: marvell: neta: disable comphy when setting mode
From: Marek Behún @ 2019-02-25 16:43 UTC (permalink / raw)
To: netdev; +Cc: Russell King, Marek Behún
The comphy driver for Armada 3700 by Miquèl Raynal (which is currently
in linux-next) does not actually set comphy mode when phy_set_mode_ext
is called. The mode is set at next call of phy_power_on.
Update the driver to semantics similar to mvpp2: helper
mvneta_comphy_init sets comphy mode and powers it on.
When mode is to be changed in mvneta_mac_config, first power the comphy
off, then call mvneta_comphy_init (which sets the mode to new one).
Only do this when new mode is different from old mode.
This should also work for Armada 38x, since in that comphy driver
methods power_on and power_off are unimplemented.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
drivers/net/ethernet/marvell/mvneta.c | 28 ++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ff275780040f..c0a3718b2e2a 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3149,11 +3149,26 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
return 0;
}
+static int mvneta_comphy_init(struct mvneta_port *pp)
+{
+ int ret;
+
+ if (!pp->comphy)
+ return 0;
+
+ ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
+ pp->phy_interface);
+ if (ret)
+ return ret;
+
+ return phy_power_on(pp->comphy);
+}
+
static void mvneta_start_dev(struct mvneta_port *pp)
{
int cpu;
- WARN_ON(phy_power_on(pp->comphy));
+ WARN_ON(mvneta_comphy_init(pp));
mvneta_max_rx_size_set(pp, pp->pkt_size);
mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
@@ -3525,12 +3540,15 @@ static void mvneta_mac_config(struct net_device *ndev, unsigned int mode,
if (state->speed == SPEED_2500)
new_ctrl4 |= MVNETA_GMAC4_SHORT_PREAMBLE_ENABLE;
- if (pp->comphy &&
+ if (pp->comphy && pp->phy_interface != state->interface &&
(state->interface == PHY_INTERFACE_MODE_SGMII ||
state->interface == PHY_INTERFACE_MODE_1000BASEX ||
- state->interface == PHY_INTERFACE_MODE_2500BASEX))
- WARN_ON(phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
- state->interface));
+ state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
+ pp->phy_interface = state->interface;
+
+ WARN_ON(phy_power_off(pp->comphy));
+ WARN_ON(mvneta_comphy_init(pp));
+ }
if (new_ctrl0 != gmac_ctrl0)
mvreg_write(pp, MVNETA_GMAC_CTRL_0, new_ctrl0);
--
2.19.2
^ permalink raw reply related
* Re: [net-next 05/14] i40e: fix up 32 bit timespec references
From: Jesse Brandeburg @ 2019-02-25 16:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jeff Kirsher, David Miller, Networking, nhorman, sassmann,
jogreene, jacob.e.keller
In-Reply-To: <CAK8P3a3fSofgDdLZ3S+MaEVz8DHnX6HO6xGiRWfCjQ614Kf4Cg@mail.gmail.com>
On Mon, 25 Feb 2019 15:46:18 +0100
Arnd Bergmann <arnd@arndb.de> wrote:
> I just stumbled over code added by this older patch, and can't make sense
> of the commit description here. Was this an attempt to fix a bug, or
> just a cleanup?
Thanks Arnd for mentioning/posting this! This was an attempt at a clean
up, unfortunately I made a mistake which introduced a bug.
> > - then = ns_to_timespec64(delta);
> > mutex_lock(&pf->tmreg_lock);
> >
> > i40e_ptp_read(pf, &now);
> > - now = timespec64_add(now, then);
> > + timespec64_add_ns(&now, delta);
> > i40e_ptp_write(pf, (const struct timespec64 *)&now);
>
> The problem I noticed here is that 'delta' is a user provided 64-bit
> number from clock_adjtime(), and timespec64_add_ns() performs uses
> a repeated addition instead of a div/mod pair. When the number
> is large, we may end up adding a single second 8 billion times,
> which may take a while even on a fast CPU.
>
> Should the commit 0ac30ce43323 ("i40e: fix up 32 bit timespec
> references") just be reverted?
Interestingly, we just were discussing this fix this past Friday, and
have a patch internally to fix the issue correctly. Either way it
is a commit (revert or fix) so we might as well just send the fix ASAP.
Our team should have something posted to intel-wired-lan/netdev shortly,
and we'll get the patch officially to netdev after a brief test.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 4/9] bpf: add bpf helper bpf_skb_ecn_set_ce
From: Eric Dumazet @ 2019-02-25 16:52 UTC (permalink / raw)
To: Daniel Borkmann, Martin Lau
Cc: Lawrence Brakmo, netdev, Alexei Starovoitov, Kernel Team
In-Reply-To: <c9ebe42a-4fc5-74f5-1b80-30ec45772174@iogearbox.net>
On 02/25/2019 02:10 AM, Daniel Borkmann wrote:
> My understanding is that before doing any writes into skb, we should make
> sure the data area is private to us (and offset in linear data). In tc BPF
> (ingress, egress) we use bpf_try_make_writable() helper for this, others
> like act_{pedit,skbmod} or ovs have similar logic before writing into skb,
> note that in all these cases it's mostly about generic writes, so location
> could also be L4, for example.
>
> Difference of above helper compared to net/sched/sch_*.c instances could
> be that it's i) for the qdisc case it's only on egress INET_ECN_set_ce()
> and that there may be a convention that qdiscs specifically may mangle
> it whereas the helper could be called on ingress and egress and confuse
> other subsystems since they won't see original or race by seeing partially
> updated (invalid) packet.
>
> Eric, have a chance to clarify? Perhaps then would make sense to disallow
> the helper in cgroup ingress path.
Good observations Daniel, thanks for bringing this up.
skb_ensure_writable() seems a big hammer for the case we change some bits in IP header.
TCP cloned packets certainly can have their headers mangled, so maybe
we need to use something using skb_header_cloned() instead of skb_cloned()
^ permalink raw reply
* Re: INFO: task hung in rtnetlink_rcv_msg
From: Joe Perches @ 2019-02-25 16:55 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Daniel Borkmann
Cc: Jakub Kicinski, syzbot, ast, christian, davem, dsahern, hawk,
idosch, john.fastabend, kafai, ktkhai, linux-kernel, netdev,
petrm, roopa, songliubraving, syzkaller-bugs, xdp-newbies, yhs
In-Reply-To: <20190225093933.748ceab4@carbon>
On Mon, 2019-02-25 at 09:39 +0100, Jesper Dangaard Brouer wrote:
> On Sat, 23 Feb 2019 01:47:00 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> > On 02/22/2019 10:45 PM, Jakub Kicinski wrote:
> > > On Fri, 22 Feb 2019 12:14:57 -0800, Joe Perches wrote:
> > > > On Fri, 2019-02-22 at 12:01 -0800, Jakub Kicinski wrote:
> > > > > Hi!
> > > > >
> > > > > Seems like something funny is going on with get_maintainer.pl since XDP
> > > > > entry got added. We seem to have been CCed on:
> > > >
> > > > I suggest removing the MAINTAINERS line with
> > > >
> > > > K: xdp
> > > >
> > > > as xdp is a pretty generic file/patch content
> > > > regex match for the K: type
> > > >
> > > > $ git grep --name-only xdp | wc -l
> > > > 236
>
> I'm unsure how K: works, but you grep definitely selects some wrong files.
>
> I tried with "xdp_":
> git grep --name-only xdp_
>
> That does catch all the driver that have XDP support, which is the
> point of the exercise (to catch drivers).
>
> It does contain a couple of false-positives:
> drivers/net/ethernet/neterion/vxge/vxge-traffic.c
> drivers/thunderbolt/tb_msgs.h
> drivers/thunderbolt/xdomain.c
> sound/soc/codecs/rt5670.c
>
> Via the pattern '[^a-z]xdp_' I'm only left with the thunderbolt
> false-positive, as it have a data struct's called tb_xdp_*.
Then it's probably better to use
K: \Wxdp_
and if XDP_ is also to be matched, maybe
K: (?i:\Wxdp_)
^ permalink raw reply
* Re: [PATCH net] geneve: use ipv6_mod_enabled to check if ipv6 is disabled
From: David Ahern @ 2019-02-25 16:57 UTC (permalink / raw)
To: Hangbin Liu, netdev; +Cc: Stefano Brivio, David S . Miller
In-Reply-To: <20190225041350.9913-1-liuhangbin@gmail.com>
On 2/24/19 9:13 PM, Hangbin Liu wrote:
> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
> at running time.
>
> Fixes: c0a47e44c098 ("geneve: should not call rt6_lookup() when ipv6 was disabled")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> drivers/net/geneve.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 3377ac66a347..42c5e43ece68 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1514,7 +1514,7 @@ static void geneve_link_config(struct net_device *dev,
> case AF_INET6: {
> struct rt6_info *rt;
>
> - if (!__in6_dev_get(dev))
> + if (!ipv6_mod_enabled())
> break;
>
> rt = rt6_lookup(geneve->net, &info->key.u.ipv6.dst, NULL, 0,
>
seems to me the existing check is more appropriate and relevant for the
code path: is ipv6 enabled on this device versus is ipv6 enabled at all.
^ 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