* Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Make VTU miss violations less spammy
From: Florian Fainelli @ 2018-03-28 18:11 UTC (permalink / raw)
To: Andrew Lunn, David Miller; +Cc: netdev
In-Reply-To: <1522187980-23072-3-git-send-email-andrew@lunn.ch>
On 03/27/2018 02:59 PM, Andrew Lunn wrote:
> VTU miss violations can happen under normal conditions. Don't spam the
> kernel log. The statistics counter will indicate it is happening, if
> anybody is interested.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/dsa/mv88e6xxx/global1_vtu.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> index 2cbaf946e7ed..e0f1b4f6e29f 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> @@ -547,11 +547,9 @@ static irqreturn_t mv88e6xxx_g1_vtu_prob_irq_thread_fn(int irq, void *dev_id)
> chip->ports[spid].vtu_member_violation++;
> }
>
> - if (val & MV88E6XXX_G1_VTU_OP_MISS_VIOLATION) {
> - dev_err_ratelimited(chip->dev, "VTU miss violation for vid %d, source port %d\n",
> - entry.vid, spid);
Why not keep it as a dev_dbg() message? Ideally we would want to keep
those message around when the port is enslaved to a bridge, and vlan
filtering is enabled. In other cases, I agree this is just spam with the
current error level.
> + if (val & MV88E6XXX_G1_VTU_OP_MISS_VIOLATION)
> chip->ports[spid].vtu_miss_violation++;
> - }
> +
> mutex_unlock(&chip->reg_lock);
>
> return IRQ_HANDLED;
>
--
Florian
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 18:10 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <80d7af37-10ef-28d7-f03f-27b4b5849cd1@fb.com>
On Wed, 28 Mar 2018 11:03:24 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> I can live with this overhead if Mathieu insists,
> but I prefer to keep it in 'struct tracepoint'.
>
> Thoughts?
I'm fine with keeping it as is. We could probably use it for future
enhancements in perf and ftrace.
Perhaps, we should just add a:
#ifdef CONFIG_BPF_EVENTS
Around the use cases of num_args.
-- Steve
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 18:03 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328133830.3d5d74d3@gandalf.local.home>
On 3/28/18 10:38 AM, Steven Rostedt wrote:
> On Wed, 28 Mar 2018 10:10:34 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>
>>> and have:
>>>
>>> u64 tp_offset = (u64)tp - (u64)_sdata;
>>>
>>> if (WARN_ON(tp_offset > UINT_MAX)
>>> return -EINVAL;
>>>
>>> btp->tp_offset = (u32)tp_offset;
>>
>> above math has to be build time constant, so warn_on likely
>> won't work.
>
> Right, it would require a BUILD_BUG_ON.
>
>> imo the whole thing is too fragile and obscure.
>> I suggest to compress this 8 bytes * num_of_tracepoints later.
>> Especially would be good to do it in one way for
>> bpf_raw_event_map, ftrace and other places.
>
> Fair enough. We can defer this shrinkage to another time. I only
> suggested it here over your concern for the added bloat.
Actually, I will take it back.
I think the current shape of the patch is better.
struct tracepoint is aligned to 32-bytes by linker
Though sizeof(struct tracepoint) == 48 it actually consumes 64 bytes
of memory.
(gdb) p (void*)&__tracepoint_sys_enter - (void*)&__tracepoint_sys_exit
$3 = 64
Adding num_args to 'struct tracepoint' makes it sizeof==56,
but it still takes 64-bytes in memory.
In this patch sizeof(struct bpf_raw_tp_map) == 16 and
(gdb) p (void*)&__bpf_trace_tp_map_sys_enter -
(void*)&__bpf_trace_tp_map_sys_exit
$3 = 16
so it consumes exactly the same 16-bytes.
If we add 'u32 num_args' to it, it will have
sizeof(struct bpf_raw_tp_map) == 24 and will consume 32-bytes
of memory.
So clearly adding num_args to struct tracepont gives zero additional
bloat vs before this patch set, whereas moving it to
struct bpf_raw_tp_map will add 16 * num_of_tracepoints bytes of memory.
I can live with this overhead if Mathieu insists,
but I prefer to keep it in 'struct tracepoint'.
Thoughts?
^ permalink raw reply
* Re: NFS mounts failing when keytab present on client
From: J. Bruce Fields @ 2018-03-28 18:04 UTC (permalink / raw)
To: Eric Biggers
Cc: Michael Young, Herbert Xu, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <20180328180334.GB3354@fieldses.org>
On Wed, Mar 28, 2018 at 02:03:34PM -0400, J. Bruce Fields wrote:
> Thanks, got it. Do you know how to find a commit id for that change?
> It's not entirely fair to blame the crypto change for what was really a
> latent nfs bug, but it might still be worth adding a Fixes: line just so
> people know where it needs backporting.
Whoops, I see you already did that. I should finish my inbox before
responding!
--b.
^ permalink raw reply
* Re: NFS mounts failing when keytab present on client
From: J. Bruce Fields @ 2018-03-28 18:03 UTC (permalink / raw)
To: Eric Biggers
Cc: Michael Young, Herbert Xu, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <20180328175051.GB185597@google.com>
On Wed, Mar 28, 2018 at 10:50:51AM -0700, Eric Biggers wrote:
> On Wed, Mar 28, 2018 at 11:46:28AM -0400, J. Bruce Fields wrote:
> > On Tue, Mar 27, 2018 at 03:29:50PM -0700, Eric Biggers wrote:
> > > Hi Michael,
> > >
> > > On Tue, Mar 27, 2018 at 11:06:14PM +0100, Michael Young wrote:
> > > > NFS mounts stopped working on one of my computers after a kernel update from
> > > > 4.15.3 to 4.15.4. I traced the problem to the commit
> > > > [46e8d06e423c4f35eac7a8b677b713b3ec9b0684] crypto: hash - prevent using
> > > > keyed hashes without setting key
> > > > and a later kernel with this patch reverted works normally.
> > > >
> > > > The problem seems to be related to kerberos as the mount fails when the
> > > > keytab is present, but works if I rename the keytab file. This is true even
> > > > though the mount is with sec=sys . The mount should also work with sec=krb5
> > > > but that also fails in the same way. When the mount fails there are errors
> > > > in dmesg like
> > > > [ 1232.522816] gss_marshal: gss_get_mic FAILED (851968)
> > > > [ 1232.522819] RPC: couldn't encode RPC header, exit EIO
> > > > [ 1232.522856] gss_marshal: gss_get_mic FAILED (851968)
> > > > [ 1232.522857] RPC: couldn't encode RPC header, exit EIO
> > > > [ 1232.522863] NFS: nfs4_discover_server_trunking unhandled error -5.
> > > > Exiting with error EIO
> > > > [ 1232.525039] gss_marshal: gss_get_mic FAILED (851968)
> > > > [ 1232.525042] RPC: couldn't encode RPC header, exit EIO
> > > >
> > > > Michael Young
> > >
> > > Thanks for the bug report. I think the error is coming from
> > > net/sunrpc/auth_gss/gss_krb5_crypto.c. There are two potential problems I see.
> > > The first one, which is definitely a bug, is that make_checksum_hmac_md5()
> > > allocates an HMAC transform and request, then does these crypto API calls:
> > >
> > > crypto_ahash_init()
> > > crypto_ahash_setkey()
> > > crypto_ahash_digest()
> > >
> > > This is wrong because it makes no sense to init() the HMAC request before the
> > > key has been set, and doubly so when it's calling digest() which is shorthand
> > > for init() + update() + final(). So I think it just needs to be removed. You
> > > can test the following patch:
> >
> > When was this introduced?
> >
> > 3b5cf20cf439 "sunrpc: Use skcipher and ahash/shash"
> > - probably not, assuming the above was still just as wrong with
> > crypto_hash_{init,setkey,digest} as it is with
> > crypto_ahash_{init,setkey,digest}
> >
> > So I'm guessing it was wrong from the start when it was added by
> > fffdaef2eb4a "gss_krb5: Add support for rc4-hmac encryption" 8 years
> > ago. Wonder why it took this long to notice? Did something else
> > change?
> >
> > --b.
>
> It was wrong from the start, but the crypto API only recently started enforcing
> that the key has to be set before init() or digest() is called. Before that the
> code was just doing unnecessary work, at least with the software HMAC
> implementation. Though, there are also hardware crypto drivers that implement
> HMAC-MD5, and it's not immediately obvious that they handle init() before
> setkey() as gracefully as the software implementation.
Thanks, got it. Do you know how to find a commit id for that change?
It's not entirely fair to blame the crypto change for what was really a
latent nfs bug, but it might still be worth adding a Fixes: line just so
people know where it needs backporting.
--b.
^ permalink raw reply
* [PATCH] sunrpc: remove incorrect HMAC request initialization
From: Eric Biggers @ 2018-03-28 17:57 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, J . Bruce Fields, Jeff Layton,
linux-nfs
Cc: netdev, linux-crypto, Herbert Xu, Michael Young, Eric Biggers,
stable
make_checksum_hmac_md5() is allocating an HMAC transform and doing
crypto API calls in the following order:
crypto_ahash_init()
crypto_ahash_setkey()
crypto_ahash_digest()
This is wrong because it makes no sense to init() the request before a
key has been set, given that the initial state depends on the key. And
digest() is short for init() + update() + final(), so in this case
there's no need to explicitly call init() at all.
Before commit 9fa68f620041 ("crypto: hash - prevent using keyed hashes
without setting key") the extra init() had no real effect, at least for
the software HMAC implementation. (There are also hardware drivers that
implement HMAC-MD5, and it's not immediately obvious how gracefully they
handle init() before setkey().) But now the crypto API detects this
incorrect initialization and returns -ENOKEY. This is breaking NFS
mounts in some cases.
Fix it by removing the incorrect call to crypto_ahash_init().
Reported-by: Michael Young <m.a.young@durham.ac.uk>
Fixes: 9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")
Fixes: fffdaef2eb4a ("gss_krb5: Add support for rc4-hmac encryption")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 12649c9fedab..8654494b4d0a 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -237,9 +237,6 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
- err = crypto_ahash_init(req);
- if (err)
- goto out;
err = crypto_ahash_setkey(hmac_md5, cksumkey, kctx->gk5e->keylength);
if (err)
goto out;
--
2.17.0.rc1.321.gba9d0f2565-goog
^ permalink raw reply related
* Re: NFS mounts failing when keytab present on client
From: Eric Biggers @ 2018-03-28 17:50 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Michael Young, Herbert Xu, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <20180328154628.GA3038@fieldses.org>
On Wed, Mar 28, 2018 at 11:46:28AM -0400, J. Bruce Fields wrote:
> On Tue, Mar 27, 2018 at 03:29:50PM -0700, Eric Biggers wrote:
> > Hi Michael,
> >
> > On Tue, Mar 27, 2018 at 11:06:14PM +0100, Michael Young wrote:
> > > NFS mounts stopped working on one of my computers after a kernel update from
> > > 4.15.3 to 4.15.4. I traced the problem to the commit
> > > [46e8d06e423c4f35eac7a8b677b713b3ec9b0684] crypto: hash - prevent using
> > > keyed hashes without setting key
> > > and a later kernel with this patch reverted works normally.
> > >
> > > The problem seems to be related to kerberos as the mount fails when the
> > > keytab is present, but works if I rename the keytab file. This is true even
> > > though the mount is with sec=sys . The mount should also work with sec=krb5
> > > but that also fails in the same way. When the mount fails there are errors
> > > in dmesg like
> > > [ 1232.522816] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522819] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522856] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522857] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522863] NFS: nfs4_discover_server_trunking unhandled error -5.
> > > Exiting with error EIO
> > > [ 1232.525039] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.525042] RPC: couldn't encode RPC header, exit EIO
> > >
> > > Michael Young
> >
> > Thanks for the bug report. I think the error is coming from
> > net/sunrpc/auth_gss/gss_krb5_crypto.c. There are two potential problems I see.
> > The first one, which is definitely a bug, is that make_checksum_hmac_md5()
> > allocates an HMAC transform and request, then does these crypto API calls:
> >
> > crypto_ahash_init()
> > crypto_ahash_setkey()
> > crypto_ahash_digest()
> >
> > This is wrong because it makes no sense to init() the HMAC request before the
> > key has been set, and doubly so when it's calling digest() which is shorthand
> > for init() + update() + final(). So I think it just needs to be removed. You
> > can test the following patch:
>
> When was this introduced?
>
> 3b5cf20cf439 "sunrpc: Use skcipher and ahash/shash"
> - probably not, assuming the above was still just as wrong with
> crypto_hash_{init,setkey,digest} as it is with
> crypto_ahash_{init,setkey,digest}
>
> So I'm guessing it was wrong from the start when it was added by
> fffdaef2eb4a "gss_krb5: Add support for rc4-hmac encryption" 8 years
> ago. Wonder why it took this long to notice? Did something else
> change?
>
> --b.
It was wrong from the start, but the crypto API only recently started enforcing
that the key has to be set before init() or digest() is called. Before that the
code was just doing unnecessary work, at least with the software HMAC
implementation. Though, there are also hardware crypto drivers that implement
HMAC-MD5, and it's not immediately obvious that they handle init() before
setkey() as gracefully as the software implementation.
Eric
^ permalink raw reply
* Re: NFS mounts failing when keytab present on client
From: Eric Biggers @ 2018-03-28 17:47 UTC (permalink / raw)
To: M A Young
Cc: Herbert Xu, J. Bruce Fields, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <alpine.LFD.2.21.1803280857200.2500@algedi.dur.ac.uk>
On Wed, Mar 28, 2018 at 09:00:14AM +0100, M A Young wrote:
> On Tue, 27 Mar 2018, Eric Biggers wrote:
>
> > Hi Michael,
> >
> > On Tue, Mar 27, 2018 at 11:06:14PM +0100, Michael Young wrote:
> > > NFS mounts stopped working on one of my computers after a kernel update from
> > > 4.15.3 to 4.15.4. I traced the problem to the commit
> > > [46e8d06e423c4f35eac7a8b677b713b3ec9b0684] crypto: hash - prevent using
> > > keyed hashes without setting key
> > > and a later kernel with this patch reverted works normally.
> > >
> > > The problem seems to be related to kerberos as the mount fails when the
> > > keytab is present, but works if I rename the keytab file. This is true even
> > > though the mount is with sec=sys . The mount should also work with sec=krb5
> > > but that also fails in the same way. When the mount fails there are errors
> > > in dmesg like
> > > [ 1232.522816] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522819] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522856] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.522857] RPC: couldn't encode RPC header, exit EIO
> > > [ 1232.522863] NFS: nfs4_discover_server_trunking unhandled error -5.
> > > Exiting with error EIO
> > > [ 1232.525039] gss_marshal: gss_get_mic FAILED (851968)
> > > [ 1232.525042] RPC: couldn't encode RPC header, exit EIO
> > >
> > > Michael Young
> >
> > Thanks for the bug report. I think the error is coming from
> > net/sunrpc/auth_gss/gss_krb5_crypto.c. There are two potential problems I see.
> > The first one, which is definitely a bug, is that make_checksum_hmac_md5()
> > allocates an HMAC transform and request, then does these crypto API calls:
> >
> > crypto_ahash_init()
> > crypto_ahash_setkey()
> > crypto_ahash_digest()
> >
> > This is wrong because it makes no sense to init() the HMAC request before the
> > key has been set, and doubly so when it's calling digest() which is shorthand
> > for init() + update() + final(). So I think it just needs to be removed. You
> > can test the following patch:
> >
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> > index 12649c9fedab..8654494b4d0a 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> > @@ -237,9 +237,6 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
> >
> > ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
> >
> > - err = crypto_ahash_init(req);
> > - if (err)
> > - goto out;
> > err = crypto_ahash_setkey(hmac_md5, cksumkey, kctx->gk5e->keylength);
> > if (err)
> > goto out;
> >
> > If that's not it, it's also possible that the error is coming from the
> > crypto_ahash_init() in make_checksum(). That can only happen if 'cksumkey' is
> > NULL and the hash algorithm is keyed, which implies a logical error as it
> > doesn't make sense to use a keyed hash algorithm without the key. The callers
> > do check kctx->gk5e->keyed_cksum which I'd hope would prevent this, though
> > perhaps kctx->cksum can be NULL.
> >
> > Eric
>
> The patch fixes the problem.
>
> Michael Young
Okay, thanks for testing! I'll send a formal patch.
Eric
^ permalink raw reply
* Re: [PATCH][next] wil6210: fix potential null dereference of ndev before null check
From: merez @ 2018-03-28 17:46 UTC (permalink / raw)
To: Colin King
Cc: Kalle Valo, linux-wireless, wil6210, netdev, kernel-janitors,
linux-kernel, linux-wireless-owner
In-Reply-To: <20180328174027.31551-1-colin.king@canonical.com>
On 2018-03-28 20:40, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer ndev is being dereferenced before it is being null checked,
> hence there is a potential null pointer deference. Fix this by only
> dereferencing ndev after it has been null checked
>
> Detected by CoverityScan, CID#1467010 ("Dereference before null check")
>
> Fixes: e00243fab84b ("wil6210: infrastructure for multiple virtual
> interfaces")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wireless/ath/wil6210/main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/main.c
> b/drivers/net/wireless/ath/wil6210/main.c
> index a4b413e8d55a..82aec6b06d09 100644
> --- a/drivers/net/wireless/ath/wil6210/main.c
> +++ b/drivers/net/wireless/ath/wil6210/main.c
> @@ -391,7 +391,7 @@ static void wil_fw_error_worker(struct work_struct
> *work)
> struct wil6210_priv *wil = container_of(work, struct wil6210_priv,
> fw_error_worker);
> struct net_device *ndev = wil->main_ndev;
> - struct wireless_dev *wdev = ndev->ieee80211_ptr;
> + struct wireless_dev *wdev;
>
> wil_dbg_misc(wil, "fw error worker\n");
>
> @@ -399,6 +399,7 @@ static void wil_fw_error_worker(struct work_struct
> *work)
> wil_info(wil, "No recovery - interface is down\n");
> return;
> }
> + wdev = ndev->ieee80211_ptr;
>
> /* increment @recovery_count if less then WIL6210_FW_RECOVERY_TO
> * passed since last recovery attempt
Reviewed-by: Maya Erez <merez@codeaurora.org>
--
Maya Erez
Qualcomm Israel, Inc. on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH bpf-next]: add sock_ops R/W access to ipv4 tos
From: Nikita Shirokov @ 2018-03-28 17:41 UTC (permalink / raw)
To: Daniel Borkmann, Lawrence Brakmo, ast@kernel.org,
netdev@vger.kernel.org
Cc: Kernel Team
In-Reply-To: <67c87527-6f6c-d5fb-5fe9-83c3aa1d000d@iogearbox.net>
>>On 03/26/2018 05:36 PM, Nikita V. Shirokov wrote:
>> bpf: Add sock_ops R/W access to ipv4 tos
>>
>> Sample usage for tos:
>>
>> bpf_getsockopt(skops, SOL_IP, IP_TOS, &v, sizeof(v))
>>
>> where skops is a pointer to the ctx (struct bpf_sock_ops).
>>
>> Signed-off-by: Nikita V. Shirokov <tehnerd@fb.com>
>> ---
>> net/core/filter.c | 35 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 00c711c..afd8255 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -3462,6 +3462,27 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>> ret = -EINVAL;
>> }
>> #ifdef CONFIG_INET
>> + } else if (level == SOL_IP) {
>> + if (optlen != sizeof(int) || sk->sk_family != AF_INET)
>> + return -EINVAL;
>> +
>> + val = *((int *)optval);
>> + /* Only some options are supported */
>> + switch (optname) {
>> + case IP_TOS:
>> + if (val < -1 || val > 0xff) {
>> + ret = -EINVAL;
>> + } else {
>> + struct inet_sock *inet = inet_sk(sk);
>> +
>> + if (val == -1)
>> + val = 0;
>> + inet->tos = val;
>
>Should this not have the exact same semantics given the helper resembles
>the normal setsockopt? do_ip_setsockopt() does the following when setting
>IP_TOS:
>
> case IP_TOS: /* This sets both TOS and Precedence */
> if (sk->sk_type == SOCK_STREAM) {
> val &= ~INET_ECN_MASK;
> val |= inet->tos & INET_ECN_MASK;
> }
> if (inet->tos != val) {
> inet->tos = val;
> sk->sk_priority = rt_tos2priority(val);
> sk_dst_reset(sk);
> }
> break;
>
>E.g. why we don't need to set sk->sk_priority as well or reset the dst
>entry here?
>
it feels like initially (w/ commit for IP_TOS in ip_sockglue.c) there were some usecase in mind
where reflection of tos to prio was needed + some policy based routing (thats why dst_reset).
but e.g. for ipv6 (IPV6_TCLASS, same as TOS but in ipv6 world) we do just this - set new tclass value
and call it the day. in my opinion this aproach is more flexible (e.g. we have separate
bpf_setsockopt for SOL_PRIORITY) as it did only what we want (i can imagine few usecases
where we want just to change TOS w/o changing priority)
however if you feel strong and want to reproduce the same behavior as in regular setsockopt, i can
totally make v2. please let me know
>> + }
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> #if IS_ENABLED(CONFIG_IPV6)
>> } else if (level == SOL_IPV6) {
>> if (optlen != sizeof(int) || sk->sk_family != AF_INET6)
>> @@ -3561,6 +3582,20 @@ BPF_CALL_5(bpf_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>> } else {
>> goto err_clear;
>> }
>> + } else if (level == SOL_IP) {
>> + struct inet_sock *inet = inet_sk(sk);
>> +
>> + if (optlen != sizeof(int) || sk->sk_family != AF_INET)
>> + goto err_clear;
>> +
>> + /* Only some options are supported */
>> + switch (optname) {
>> + case IP_TOS:
>> + *((int *)optval) = (int)inet->tos;
>
>This part is fine though, same as in do_ip_getsockopt().
>
>> + break;
>> + default:
>> + goto err_clear;
>> + }
>> #if IS_ENABLED(CONFIG_IPV6)
>> } else if (level == SOL_IPV6) {
>> struct ipv6_pinfo *np = inet6_sk(sk);
>>
^ permalink raw reply
* Re: [PATCH v7 bpf-next 07/10] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-28 17:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev,
kernel-team, linux-api
In-Reply-To: <20180328021105.4061744-8-ast@fb.com>
On Tue, 27 Mar 2018 19:11:02 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> Introduce BPF_PROG_TYPE_RAW_TRACEPOINT bpf program type to access
> kernel internal arguments of the tracepoints in their raw form.
>
> >From bpf program point of view the access to the arguments look like:
> struct bpf_raw_tracepoint_args {
> __u64 args[0];
> };
>
> int bpf_prog(struct bpf_raw_tracepoint_args *ctx)
> {
> // program can read args[N] where N depends on tracepoint
> // and statically verified at program load+attach time
> }
>
> kprobe+bpf infrastructure allows programs access function arguments.
> This feature allows programs access raw tracepoint arguments.
>
> Similar to proposed 'dynamic ftrace events' there are no abi guarantees
> to what the tracepoints arguments are and what their meaning is.
> The program needs to type cast args properly and use bpf_probe_read()
> helper to access struct fields when argument is a pointer.
>
> For every tracepoint __bpf_trace_##call function is prepared.
> In assembler it looks like:
> (gdb) disassemble __bpf_trace_xdp_exception
> Dump of assembler code for function __bpf_trace_xdp_exception:
> 0xffffffff81132080 <+0>: mov %ecx,%ecx
> 0xffffffff81132082 <+2>: jmpq 0xffffffff811231f0 <bpf_trace_run3>
>
> where
>
> TRACE_EVENT(xdp_exception,
> TP_PROTO(const struct net_device *dev,
> const struct bpf_prog *xdp, u32 act),
>
> The above assembler snippet is casting 32-bit 'act' field into 'u64'
> to pass into bpf_trace_run3(), while 'dev' and 'xdp' args are passed as-is.
> All of ~500 of __bpf_trace_*() functions are only 5-10 byte long
> and in total this approach adds 7k bytes to .text.
>
> This approach gives the lowest possible overhead
> while calling trace_xdp_exception() from kernel C code and
> transitioning into bpf land.
> Since tracepoint+bpf are used at speeds of 1M+ events per second
> this is valuable optimization.
>
> The new BPF_RAW_TRACEPOINT_OPEN sys_bpf command is introduced
> that returns anon_inode FD of 'bpf-raw-tracepoint' object.
>
> The user space looks like:
> // load bpf prog with BPF_PROG_TYPE_RAW_TRACEPOINT type
> prog_fd = bpf_prog_load(...);
> // receive anon_inode fd for given bpf_raw_tracepoint with prog attached
> raw_tp_fd = bpf_raw_tracepoint_open("xdp_exception", prog_fd);
>
> Ctrl-C of tracing daemon or cmdline tool that uses this feature
> will automatically detach bpf program, unload it and
> unregister tracepoint probe.
>
> On the kernel side the __bpf_raw_tp_map section of pointers to
> tracepoint definition and to __bpf_trace_*() probe function is used
> to find a tracepoint with "xdp_exception" name and
> corresponding __bpf_trace_xdp_exception() probe function
> which are passed to tracepoint_probe_register() to connect probe
> with tracepoint.
>
> Addition of bpf_raw_tracepoint doesn't interfere with ftrace and perf
> tracepoint mechanisms. perf_event_open() can be used in parallel
> on the same tracepoint.
> Multiple bpf_raw_tracepoint_open("xdp_exception", prog_fd) are permitted.
> Each with its own bpf program. The kernel will execute
> all tracepoint probes and all attached bpf programs.
>
> In the future bpf_raw_tracepoints can be extended with
> query/introspection logic.
>
> __bpf_raw_tp_map section logic was contributed by Steven Rostedt
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
Just an FYI, I applied all the patches up to and including this one
(made sure BPF_EVENTS was enabled in my config this time), built and
booted the kernel and ran a bunch of tests (not my full suite, but
enough).
It didn't affect any other tracing features that I can see.
-- Steve
^ permalink raw reply
* [PATCH][next] wil6210: fix potential null dereference of ndev before null check
From: Colin King @ 2018-03-28 17:40 UTC (permalink / raw)
To: Maya Erez, Kalle Valo, linux-wireless, wil6210, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The pointer ndev is being dereferenced before it is being null checked,
hence there is a potential null pointer deference. Fix this by only
dereferencing ndev after it has been null checked
Detected by CoverityScan, CID#1467010 ("Dereference before null check")
Fixes: e00243fab84b ("wil6210: infrastructure for multiple virtual interfaces")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/ath/wil6210/main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wil6210/main.c b/drivers/net/wireless/ath/wil6210/main.c
index a4b413e8d55a..82aec6b06d09 100644
--- a/drivers/net/wireless/ath/wil6210/main.c
+++ b/drivers/net/wireless/ath/wil6210/main.c
@@ -391,7 +391,7 @@ static void wil_fw_error_worker(struct work_struct *work)
struct wil6210_priv *wil = container_of(work, struct wil6210_priv,
fw_error_worker);
struct net_device *ndev = wil->main_ndev;
- struct wireless_dev *wdev = ndev->ieee80211_ptr;
+ struct wireless_dev *wdev;
wil_dbg_misc(wil, "fw error worker\n");
@@ -399,6 +399,7 @@ static void wil_fw_error_worker(struct work_struct *work)
wil_info(wil, "No recovery - interface is down\n");
return;
}
+ wdev = ndev->ieee80211_ptr;
/* increment @recovery_count if less then WIL6210_FW_RECOVERY_TO
* passed since last recovery attempt
--
2.15.1
^ permalink raw reply related
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 17:38 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <fdcb0bf0-f896-ffb3-b9de-0b17f5f70fc6@fb.com>
On Wed, 28 Mar 2018 10:10:34 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> > and have:
> >
> > u64 tp_offset = (u64)tp - (u64)_sdata;
> >
> > if (WARN_ON(tp_offset > UINT_MAX)
> > return -EINVAL;
> >
> > btp->tp_offset = (u32)tp_offset;
>
> above math has to be build time constant, so warn_on likely
> won't work.
Right, it would require a BUILD_BUG_ON.
> imo the whole thing is too fragile and obscure.
> I suggest to compress this 8 bytes * num_of_tracepoints later.
> Especially would be good to do it in one way for
> bpf_raw_event_map, ftrace and other places.
Fair enough. We can defer this shrinkage to another time. I only
suggested it here over your concern for the added bloat.
-- Steve
^ permalink raw reply
* Re: [PATCH v2 net-next] qed*: Utilize FW 8.33.11.0
From: Jason Gunthorpe @ 2018-03-28 17:32 UTC (permalink / raw)
To: Michal Kalderon
Cc: davem, netdev, linux-rdma, linux-scsi, Tomer Tayar,
Manish Rangankar, Ariel Elior
In-Reply-To: <1522226536-9812-1-git-send-email-Michal.Kalderon@cavium.com>
On Wed, Mar 28, 2018 at 11:42:16AM +0300, Michal Kalderon wrote:
> This FW contains several fixes and features
>
> RDMA Features
> - SRQ support
> - XRC support
> - Memory window support
> - RDMA low latency queue support
> - RDMA bonding support
>
> RDMA bug fixes
> - RDMA remote invalidate during retransmit fix
> - iWARP MPA connect interop issue with RTR fix
> - iWARP Legacy DPM support
> - Fix MPA reject flow
> - iWARP error handling
> - RQ WQE validation checks
>
> MISC
> - Fix some HSI types endianity
> - New Restriction: vlan insertion in core_tx_bd_data can't be set
> for LB packets
>
> ETH
> - HW QoS offload support
> - Fix vlan, dcb and sriov flow of VF sending a packet with
> inband VLAN tag instead of default VLAN
> - Allow GRE version 1 offloads in RX flow
> - Allow VXLAN steering
>
> iSCSI / FcoE
> - Fix bd availability checking flow
> - Support 256th sge proerly in iscsi/fcoe retransmit
> - Performance improvement
> - Fix handle iSCSI command arrival with AHS and with immediate
> - Fix ipv6 traffic class configuration
>
> DEBUG
> - Update debug utilities
>
> Signed-off-by: Michal Kalderon <Michal.Kalderon@cavium.com>
> Signed-off-by: Tomer Tayar <Tomer.Tayar@cavium.com>
> Signed-off-by: Manish Rangankar <Manish.Rangankar@cavium.com>
> Signed-off-by: Ariel Elior <Ariel.Elior@cavium.com>
> ---
> Changes from v1
> Remove version bump and qedr module version addition
> ---
> drivers/infiniband/hw/qedr/qedr_hsi_rdma.h | 4 +-
> drivers/infiniband/hw/qedr/verbs.c | 4 +-
> drivers/net/ethernet/qlogic/qed/qed_debug.c | 415 +++--
> drivers/net/ethernet/qlogic/qed/qed_dev.c | 4 +-
> drivers/net/ethernet/qlogic/qed/qed_hsi.h | 1892 ++++++++++----------
> .../net/ethernet/qlogic/qed/qed_init_fw_funcs.c | 103 +-
> drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 7 -
> drivers/net/ethernet/qlogic/qed/qed_l2.c | 2 +-
> drivers/net/ethernet/qlogic/qed/qed_ll2.c | 13 -
> include/linux/qed/common_hsi.h | 2 +-
> include/linux/qed/eth_common.h | 2 +-
> include/linux/qed/iscsi_common.h | 4 +-
> include/linux/qed/rdma_common.h | 2 +
> include/linux/qed/roce_common.h | 3 +
> 14 files changed, 1310 insertions(+), 1147 deletions(-)
For drivers/infiniband:
Acked-by: Jason Gunthorpe <jgg@mellanox.com>
Jason
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-28 17:14 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <67c5f9b4-b24a-2806-e8d6-8b5241c66d6e@fb.com>
----- On Mar 28, 2018, at 12:43 PM, Alexei Starovoitov ast@fb.com wrote:
> On 3/28/18 6:49 AM, Mathieu Desnoyers wrote:
>> ----- On Mar 27, 2018, at 10:11 PM, Alexei Starovoitov ast@fb.com wrote:
>>
>>> From: Alexei Starovoitov <ast@kernel.org>
>>>
>>> compute number of arguments passed into tracepoint
>>> at compile time and store it as part of 'struct tracepoint'.
>>> The number is necessary to check safety of bpf program access that
>>> is coming in subsequent patch.
>>
>>
>> Hi Alexei,
>>
>> Given that only eBPF needs this parameter count, we can move
>> it to the struct bpf_raw_event_map newly introduced by Steven,
>> right ? This would reduce bloat of struct tracepoint. For instance,
>> we don't need to keep this count around when eBPF is configured
>> out.
>
> Makes sense. That is indeed cleaner. Will respin.
>
> What I don't like though is 'bloat' argument.
> 'u32 num_args' padded to 8-byte takes exactly the same amount
> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
> The number of these structures is the same as well
> and chances that tracepoints are on while bpf is off are slim.
> More so few emails ago you said:
> "I'm perfectly fine with adding the "num_args" stuff. I think it's
> really useful. It's only the for_each_kernel_tracepoint change for
> which I'm trying to understand the rationale."
>
> I'm guessing now you found out that num_args is not useful to lttng
> and it bloats its data structures?
I'm concerned about kernel configurations with only ftrace and
perf, with eBPF disabled. In those configurations, adding ebpf-specific
fields to struct tracepoint increases the kernel image size needlessly.
LTTng is not my concern in this discussion.
I'm also concerned about adding more and more elements to struct tracepoint
in general. For instance, I'd really like to see the regfunc/unregfunc
fields go away into a different section, allowing us to reduce the size of
struct tracepoint, so we can do a better use of the CPU caches when
tracing is enabled. The "funcs" field needs to be loaded each time a
tracepoint is fired, and therefore needs to be cache-hot. Adding more
cache-cold fields in there degrades cache locality, because "hot"
cache-lines then need to hold more cache-cold data.
> It's ok to change an opinion and I'm completely fine using that as
> a real reason.
Seeing the code of the new eBPF section provided by Steven made me realize
that we could use it to hold the num_args as well, keeping all the eBPF
data nice and tidy together. So yes, this is new information that changed
my opinion on the matter.
> Will repsin, as I said. No problem.
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 17:10 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328130407.7476cf17@gandalf.local.home>
On 3/28/18 10:04 AM, Steven Rostedt wrote:
> On Wed, 28 Mar 2018 09:43:56 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>>>
>>> Given that only eBPF needs this parameter count, we can move
>>> it to the struct bpf_raw_event_map newly introduced by Steven,
>>> right ? This would reduce bloat of struct tracepoint. For instance,
>>> we don't need to keep this count around when eBPF is configured
>>> out.
>>
>> Makes sense. That is indeed cleaner. Will respin.
>>
>> What I don't like though is 'bloat' argument.
>> 'u32 num_args' padded to 8-byte takes exactly the same amount
>> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
>> The number of these structures is the same as well
>> and chances that tracepoints are on while bpf is off are slim.
>> More so few emails ago you said:
>> "I'm perfectly fine with adding the "num_args" stuff. I think it's
>> really useful. It's only the for_each_kernel_tracepoint change for
>> which I'm trying to understand the rationale."
>
> I don't really care which one it goes in. The padding bloat is the same
> for both :-/ But I wonder if we can shrink it by doing a trick that
> Josh did in one of his patches. That is, to use a 32bit offset instead
> of a direct pointer. Since you are only accessing core kernel
> tracepoints.
>
> Thus, we could have
>
> struct bpf_raw_event_map {
> u32 tp_offset;
> u32 num_args;
> void *bpf_func;
> };
>
> and have:
>
> u64 tp_offset = (u64)tp - (u64)_sdata;
>
> if (WARN_ON(tp_offset > UINT_MAX)
> return -EINVAL;
>
> btp->tp_offset = (u32)tp_offset;
above math has to be build time constant, so warn_on likely
won't work.
imo the whole thing is too fragile and obscure.
I suggest to compress this 8 bytes * num_of_tracepoints later.
Especially would be good to do it in one way for
bpf_raw_event_map, ftrace and other places.
> And to get the tp, all you need to do is:
>
> tp = (struct tracepoint *)(btp->tp_offset + (unsigned long)_sdata);
>
> I've been thinking of doing this for other parts of the tracepoints and
> ftrace code.
>
> BTW, thanks for changing your code. I really appreciate it.
>
> -- Steve
>
^ permalink raw reply
* Re: [PATCH] sfp: allow cotsworks modules
From: Joe Perches @ 2018-03-28 17:05 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Andrew Lunn, Florian Fainelli, netdev
In-Reply-To: <20180328165154.GQ10980@n2100.armlinux.org.uk>
On Wed, 2018-03-28 at 17:51 +0100, Russell King - ARM Linux wrote:
> On Wed, Mar 28, 2018 at 09:19:01AM -0700, Joe Perches wrote:
> > On Wed, 2018-03-28 at 11:41 +0100, Russell King - ARM Linux wrote:
> > > On Wed, Mar 28, 2018 at 03:33:57AM -0700, Joe Perches wrote:
> > > > On Wed, 2018-03-28 at 11:18 +0100, Russell King wrote:
> > > > > Cotsworks modules fail the checksums - it appears that Cotsworks
> > > > > reprograms the EEPROM at the end of production with the final product
> > > > > information (serial, date code, and exact part number for module
> > > > > options) and fails to update the checksum.
> > > >
> > > > trivia:
> > > >
> > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> > > >
> > > > []
> > > > > @@ -574,23 +575,43 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
> > > >
> > > > []
> > > > > + if (cotsworks) {
> > > > > + dev_warn(sfp->dev,
> > > > > + "EEPROM base structure checksum failure (0x%02x != 0x%02x)\n",
> > > > > + check, id.base.cc_base);
> > > > > + } else {
> > > > > + dev_err(sfp->dev,
> > > > > + "EEPROM base structure checksum failure: 0x%02x != 0x%02x\n",
> > > >
> > > > It'd be better to move this above the if and
> > > > use only a single format string instead of
> > > > using 2 slightly different formats.
> > >
> > > No. I think you've missed the fact that one is a _warning_ the other is
> > > an _error_ and they are emitted at the appropriate severity. It's not
> > > just that the format strings are slightly different.
> >
> > Right. Still nicer to use the same formats.
>
> I'll stick a "Warning:" and "Error:" tag before them if you really
> want the rest of the message to be identically formatted - otherwise,
> when seeing reports from people's dmesg, there will be nothing to
> indicate which message was printed.
Not necessary. It was just a trivial size saving
from the format deduplication.
There is another dmesg info line
dev_info(sfp->dev, "module %.*s %.*s rev %.*s sn %.*s dc %.*s\n",
(int)sizeof(id.base.vendor_name), id.base.vendor_name,
(int)sizeof(id.base.vendor_pn), id.base.vendor_pn,
(int)sizeof(id.base.vendor_rev), id.base.vendor_rev,
(int)sizeof(id.ext.vendor_sn), id.ext.vendor_sn,
(int)sizeof(id.ext.datecode), id.ext.datecode);
the next line later which shows that it's a "COTSWORKS ".
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 17:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <67c5f9b4-b24a-2806-e8d6-8b5241c66d6e@fb.com>
On Wed, 28 Mar 2018 09:43:56 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> >
> > Given that only eBPF needs this parameter count, we can move
> > it to the struct bpf_raw_event_map newly introduced by Steven,
> > right ? This would reduce bloat of struct tracepoint. For instance,
> > we don't need to keep this count around when eBPF is configured
> > out.
>
> Makes sense. That is indeed cleaner. Will respin.
>
> What I don't like though is 'bloat' argument.
> 'u32 num_args' padded to 8-byte takes exactly the same amount
> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
> The number of these structures is the same as well
> and chances that tracepoints are on while bpf is off are slim.
> More so few emails ago you said:
> "I'm perfectly fine with adding the "num_args" stuff. I think it's
> really useful. It's only the for_each_kernel_tracepoint change for
> which I'm trying to understand the rationale."
I don't really care which one it goes in. The padding bloat is the same
for both :-/ But I wonder if we can shrink it by doing a trick that
Josh did in one of his patches. That is, to use a 32bit offset instead
of a direct pointer. Since you are only accessing core kernel
tracepoints.
Thus, we could have
struct bpf_raw_event_map {
u32 tp_offset;
u32 num_args;
void *bpf_func;
};
and have:
u64 tp_offset = (u64)tp - (u64)_sdata;
if (WARN_ON(tp_offset > UINT_MAX)
return -EINVAL;
btp->tp_offset = (u32)tp_offset;
And to get the tp, all you need to do is:
tp = (struct tracepoint *)(btp->tp_offset + (unsigned long)_sdata);
I've been thinking of doing this for other parts of the tracepoints and
ftrace code.
BTW, thanks for changing your code. I really appreciate it.
-- Steve
^ permalink raw reply
* [iproute2-next 0/2] tipc: changes to addressing structure
From: Jon Maloy @ 2018-03-28 16:52 UTC (permalink / raw)
To: davem, netdev
Cc: mohan.krishna.ghanta.krishnamurthy, tung.q.nguyen, hoang.h.le,
jon.maloy, canh.d.luu, ying.xue, tipc-discussion
1: We introduce ability to set/get 128-bit node identities
2: We rename 'net id' to 'cluster id' in the command API,
of course in a compatible way.
3: We print out all 32-bit node addresses as an integer in hex format,
i.e., we remove the assumption about an internal structure.
Jon Maloy (2):
tipc: introduce command for handling a new 128-bit node identity
tipc: change node address printout formats
include/uapi/linux/tipc_netlink.h | 2 +
tipc/link.c | 3 +-
tipc/misc.c | 78 ++++++++++++++++++++++++++-
tipc/misc.h | 2 +
tipc/nametable.c | 16 ++----
tipc/node.c | 109 +++++++++++++++++++++++++++++++++-----
tipc/socket.c | 3 +-
7 files changed, 183 insertions(+), 30 deletions(-)
--
2.1.4
^ permalink raw reply
* [iproute2-next 2/2] tipc: change node address printout formats
From: Jon Maloy @ 2018-03-28 16:52 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion, mohan.krishna.ghanta.krishnamurthy
In-Reply-To: <1522255934-497-1-git-send-email-jon.maloy@ericsson.com>
Since a node address now per definition is only an unstructured 32-bit
integer it makes no sense print it out as a structured string.
In this commit, we replace all occurrences of "<Z.C.N>" printouts with
just an "%x".
Acked-by: GhantaKrishnamurthy MohanKrishna <mohan.krishna.ghanta.krishnamurthy@ericsson.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
tipc/link.c | 3 +--
tipc/nametable.c | 16 +++++-----------
tipc/node.c | 11 ++---------
tipc/socket.c | 3 +--
4 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/tipc/link.c b/tipc/link.c
index 4ae1c91..02f14aa 100644
--- a/tipc/link.c
+++ b/tipc/link.c
@@ -616,8 +616,7 @@ static void link_mon_print_non_applied(uint16_t applied, uint16_t member_cnt,
if (i != applied)
printf(",");
- sprintf(addr_str, "%u.%u.%u:", tipc_zone(members[i]),
- tipc_cluster(members[i]), tipc_node(members[i]));
+ sprintf(addr_str, "%x:", members[i]);
state = map_get(up_map, i) ? 'U' : 'D';
printf("%s%c", addr_str, state);
}
diff --git a/tipc/nametable.c b/tipc/nametable.c
index 770a644..2578940 100644
--- a/tipc/nametable.c
+++ b/tipc/nametable.c
@@ -26,7 +26,6 @@
static int nametable_show_cb(const struct nlmsghdr *nlh, void *data)
{
int *iteration = data;
- char port_id[PORTID_STR_LEN];
struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
struct nlattr *info[TIPC_NLA_MAX + 1] = {};
struct nlattr *attrs[TIPC_NLA_NAME_TABLE_MAX + 1] = {};
@@ -46,22 +45,17 @@ static int nametable_show_cb(const struct nlmsghdr *nlh, void *data)
return MNL_CB_ERROR;
if (!*iteration)
- printf("%-10s %-10s %-10s %-26s %-10s\n",
- "Type", "Lower", "Upper", "Port Identity",
+ printf("%-10s %-10s %-10s %-10s %-10s %-10s\n",
+ "Type", "Lower", "Upper", "Node", "Port",
"Publication Scope");
(*iteration)++;
- snprintf(port_id, sizeof(port_id), "<%u.%u.%u:%u>",
- tipc_zone(mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE])),
- tipc_cluster(mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE])),
- tipc_node(mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE])),
- mnl_attr_get_u32(publ[TIPC_NLA_PUBL_REF]));
-
- printf("%-10u %-10u %-10u %-26s %-12u",
+ printf("%-10u %-10u %-10u %-10x %-10u %-12u",
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_TYPE]),
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_LOWER]),
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_UPPER]),
- port_id,
+ mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE]),
+ mnl_attr_get_u32(publ[TIPC_NLA_PUBL_REF]),
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_KEY]));
printf("%s\n", scope[mnl_attr_get_u32(publ[TIPC_NLA_PUBL_SCOPE])]);
diff --git a/tipc/node.c b/tipc/node.c
index 3ebbe0b..b73b644 100644
--- a/tipc/node.c
+++ b/tipc/node.c
@@ -40,10 +40,7 @@ static int node_list_cb(const struct nlmsghdr *nlh, void *data)
return MNL_CB_ERROR;
addr = mnl_attr_get_u32(attrs[TIPC_NLA_NODE_ADDR]);
- printf("<%u.%u.%u>: ",
- tipc_zone(addr),
- tipc_cluster(addr),
- tipc_node(addr));
+ printf("%x: ", addr);
if (attrs[TIPC_NLA_NODE_UP])
printf("up\n");
@@ -123,11 +120,7 @@ static int cmd_node_get_addr(struct nlmsghdr *nlh, const struct cmd *cmd,
}
close(sk);
- printf("<%u.%u.%u>\n",
- tipc_zone(addr.addr.id.node),
- tipc_cluster(addr.addr.id.node),
- tipc_node(addr.addr.id.node));
-
+ printf("%x\n", addr.addr.id.node);
return 0;
}
diff --git a/tipc/socket.c b/tipc/socket.c
index 48ba821..852984e 100644
--- a/tipc/socket.c
+++ b/tipc/socket.c
@@ -84,8 +84,7 @@ static int sock_list_cb(const struct nlmsghdr *nlh, void *data)
mnl_attr_parse_nested(attrs[TIPC_NLA_SOCK_CON], parse_attrs, con);
node = mnl_attr_get_u32(con[TIPC_NLA_CON_NODE]);
- printf(" connected to <%u.%u.%u:%u>", tipc_zone(node),
- tipc_cluster(node), tipc_node(node),
+ printf(" connected to %x:%u", node,
mnl_attr_get_u32(con[TIPC_NLA_CON_SOCK]));
if (con[TIPC_NLA_CON_FLAG])
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [iproute2-next 1/2] tipc: introduce command for handling a new 128-bit node identity
From: Jon Maloy @ 2018-03-28 16:52 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion, mohan.krishna.ghanta.krishnamurthy
In-Reply-To: <1522255934-497-1-git-send-email-jon.maloy@ericsson.com>
We add the possibility to set and get a 128 bit node identifier, as
an alternative to the legacy 32-bit node address we are using now.
We also add an option to set and get 'clusterid' in the node. This
is the same as what we have so far called 'netid' and performs the
same operations. For compatibility the old 'netid' commands are
retained, -we just remove them from the help texts.
Acked-by: GhantaKrishnamurthy MohanKrishna <mohan.krishna.ghanta.krishnamurthy@ericsson.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
include/uapi/linux/tipc_netlink.h | 2 +
tipc/misc.c | 78 ++++++++++++++++++++++++++++++-
tipc/misc.h | 2 +
tipc/node.c | 98 +++++++++++++++++++++++++++++++++++++--
4 files changed, 174 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/tipc_netlink.h b/include/uapi/linux/tipc_netlink.h
index 469aa67..6bf8ec6 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -162,6 +162,8 @@ enum {
TIPC_NLA_NET_UNSPEC,
TIPC_NLA_NET_ID, /* u32 */
TIPC_NLA_NET_ADDR, /* u32 */
+ TIPC_NLA_NET_NODEID, /* u64 */
+ TIPC_NLA_NET_NODEID_W1, /* u64 */
__TIPC_NLA_NET_MAX,
TIPC_NLA_NET_MAX = __TIPC_NLA_NET_MAX - 1
diff --git a/tipc/misc.c b/tipc/misc.c
index 8091222..16849f1 100644
--- a/tipc/misc.c
+++ b/tipc/misc.c
@@ -12,7 +12,7 @@
#include <stdio.h>
#include <stdint.h>
#include <linux/tipc.h>
-
+#include <string.h>
#include "misc.h"
#define IN_RANGE(val, low, high) ((val) <= (high) && (val) >= (low))
@@ -33,3 +33,79 @@ uint32_t str2addr(char *str)
fprintf(stderr, "invalid network address \"%s\"\n", str);
return 0;
}
+
+static int is_hex(char *arr, int last)
+{
+ int i;
+
+ while (!arr[last])
+ last--;
+
+ for (i = 0; i <= last; i++) {
+ if (!IN_RANGE(arr[i], '0', '9') &&
+ !IN_RANGE(arr[i], 'a', 'f') &&
+ !IN_RANGE(arr[i], 'A', 'F'))
+ return 0;
+ }
+ return 1;
+}
+
+static int is_name(char *arr, int last)
+{
+ int i;
+ char c;
+
+ while (!arr[last])
+ last--;
+
+ if (last > 15)
+ return 0;
+
+ for (i = 0; i <= last; i++) {
+ c = arr[i];
+ if (!IN_RANGE(c, '0', '9') && !IN_RANGE(c, 'a', 'z') &&
+ !IN_RANGE(c, 'A', 'Z') && c != '-' && c != '_' &&
+ c != '.' && c != ':' && c != '@')
+ return 0;
+ }
+ return 1;
+}
+
+int str2nodeid(char *str, uint8_t *id)
+{
+ int len = strlen(str);
+ int i;
+
+ if (len > 32)
+ return -1;
+
+ if (is_name(str, len - 1)) {
+ memcpy(id, str, len);
+ return 0;
+ }
+ if (!is_hex(str, len - 1))
+ return -1;
+
+ str[len] = '0';
+ for (i = 0; i < 16; i++) {
+ if (sscanf(&str[2 * i], "%2hhx", &id[i]) != 1)
+ break;
+ }
+ return 0;
+}
+
+void nodeid2str(uint8_t *id, char *str)
+{
+ int i;
+
+ if (is_name((char *)id, 15)) {
+ memcpy(str, id, 16);
+ return;
+ }
+
+ for (i = 0; i < 16; i++)
+ sprintf(&str[2 * i], "%02x", id[i]);
+
+ for (i = 31; str[i] == '0'; i--)
+ str[i] = 0;
+}
diff --git a/tipc/misc.h b/tipc/misc.h
index 585df74..6e8afdd 100644
--- a/tipc/misc.h
+++ b/tipc/misc.h
@@ -15,5 +15,7 @@
#include <stdint.h>
uint32_t str2addr(char *str);
+int str2nodeid(char *str, uint8_t *id);
+void nodeid2str(uint8_t *id, char *str);
#endif
diff --git a/tipc/node.c b/tipc/node.c
index fe085ae..3ebbe0b 100644
--- a/tipc/node.c
+++ b/tipc/node.c
@@ -131,6 +131,90 @@ static int cmd_node_get_addr(struct nlmsghdr *nlh, const struct cmd *cmd,
return 0;
}
+static int cmd_node_set_nodeid(struct nlmsghdr *nlh, const struct cmd *cmd,
+ struct cmdl *cmdl, void *data)
+{
+ char buf[MNL_SOCKET_BUFFER_SIZE];
+ uint8_t id[16] = {0,};
+ uint64_t *w0 = (uint64_t *) &id[0];
+ uint64_t *w1 = (uint64_t *) &id[8];
+ struct nlattr *nest;
+ char *str;
+
+ if (cmdl->argc != cmdl->optind + 1) {
+ fprintf(stderr, "Usage: %s node set nodeid NODE_ID\n",
+ cmdl->argv[0]);
+ return -EINVAL;
+ }
+
+ str = shift_cmdl(cmdl);
+ if (str2nodeid(str, id)) {
+ fprintf(stderr, "Invalid node identity\n");
+ return -EINVAL;
+ }
+
+ nlh = msg_init(buf, TIPC_NL_NET_SET);
+ if (!nlh) {
+ fprintf(stderr, "error, message initialisation failed\n");
+ return -1;
+ }
+ nest = mnl_attr_nest_start(nlh, TIPC_NLA_NET);
+ mnl_attr_put_u64(nlh, TIPC_NLA_NET_NODEID, *w0);
+ mnl_attr_put_u64(nlh, TIPC_NLA_NET_NODEID_W1, *w1);
+ mnl_attr_nest_end(nlh, nest);
+ return msg_doit(nlh, NULL, NULL);
+}
+
+static int nodeid_get_cb(const struct nlmsghdr *nlh, void *data)
+{
+ struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+ struct nlattr *info[TIPC_NLA_MAX + 1] = {};
+ struct nlattr *attrs[TIPC_NLA_NET_MAX + 1] = {};
+ char str[33] = {0,};
+ uint8_t id[16] = {0,};
+ uint64_t *w0 = (uint64_t *) &id[0];
+ uint64_t *w1 = (uint64_t *) &id[8];
+ int i;
+
+ mnl_attr_parse(nlh, sizeof(*genl), parse_attrs, info);
+ if (!info[TIPC_NLA_NET])
+ return MNL_CB_ERROR;
+
+ mnl_attr_parse_nested(info[TIPC_NLA_NET], parse_attrs, attrs);
+ if (!attrs[TIPC_NLA_NET_ID])
+ return MNL_CB_ERROR;
+
+ *w0 = mnl_attr_get_u64(attrs[TIPC_NLA_NET_NODEID]);
+ *w1 = mnl_attr_get_u64(attrs[TIPC_NLA_NET_NODEID_W1]);
+ nodeid2str(id, str);
+ printf("Node Identity Hash\n");
+ printf("%s", str);
+ for (i = strlen(str); i <= 33; i++)
+ printf(" ");
+ cmd_node_get_addr(NULL, NULL, NULL, NULL);
+ return MNL_CB_OK;
+}
+
+static int cmd_node_get_nodeid(struct nlmsghdr *nlh, const struct cmd *cmd,
+ struct cmdl *cmdl, void *data)
+{
+ char buf[MNL_SOCKET_BUFFER_SIZE];
+
+ if (help_flag) {
+ (cmd->help)(cmdl);
+ return -EINVAL;
+ }
+
+ nlh = msg_init(buf, TIPC_NL_NET_GET);
+ if (!nlh) {
+ fprintf(stderr, "error, message initialisation failed\n");
+ return -1;
+ }
+
+ return msg_dumpit(nlh, nodeid_get_cb, NULL);
+}
+
+
static int netid_get_cb(const struct nlmsghdr *nlh, void *data)
{
struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
@@ -204,8 +288,8 @@ static void cmd_node_set_help(struct cmdl *cmdl)
fprintf(stderr,
"Usage: %s node set PROPERTY\n\n"
"PROPERTIES\n"
- " address ADDRESS - Set local address\n"
- " netid NETID - Set local netid\n",
+ " identity NODEID - Set node identity\n"
+ " clusterid CLUSTERID - Set local cluster id\n",
cmdl->argv[0]);
}
@@ -213,8 +297,10 @@ static int cmd_node_set(struct nlmsghdr *nlh, const struct cmd *cmd,
struct cmdl *cmdl, void *data)
{
const struct cmd cmds[] = {
- { "address", cmd_node_set_addr, NULL },
+ { "address", cmd_node_set_addr, NULL },
+ { "identity", cmd_node_set_nodeid, NULL },
{ "netid", cmd_node_set_netid, NULL },
+ { "clusterid", cmd_node_set_netid, NULL },
{ NULL }
};
@@ -226,8 +312,8 @@ static void cmd_node_get_help(struct cmdl *cmdl)
fprintf(stderr,
"Usage: %s node get PROPERTY\n\n"
"PROPERTIES\n"
- " address - Get local address\n"
- " netid - Get local netid\n",
+ " identity - Get node identity\n"
+ " clusterid - Get local clusterid\n",
cmdl->argv[0]);
}
@@ -236,7 +322,9 @@ static int cmd_node_get(struct nlmsghdr *nlh, const struct cmd *cmd,
{
const struct cmd cmds[] = {
{ "address", cmd_node_get_addr, NULL },
+ { "identity", cmd_node_get_nodeid, NULL },
{ "netid", cmd_node_get_netid, NULL },
+ { "clusterid", cmd_node_get_netid, NULL },
{ NULL }
};
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related
* Re: [PATCH] sfp: allow cotsworks modules
From: Russell King - ARM Linux @ 2018-03-28 16:51 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Lunn, Florian Fainelli, netdev
In-Reply-To: <1522253941.12357.109.camel@perches.com>
On Wed, Mar 28, 2018 at 09:19:01AM -0700, Joe Perches wrote:
> On Wed, 2018-03-28 at 11:41 +0100, Russell King - ARM Linux wrote:
> > On Wed, Mar 28, 2018 at 03:33:57AM -0700, Joe Perches wrote:
> > > On Wed, 2018-03-28 at 11:18 +0100, Russell King wrote:
> > > > Cotsworks modules fail the checksums - it appears that Cotsworks
> > > > reprograms the EEPROM at the end of production with the final product
> > > > information (serial, date code, and exact part number for module
> > > > options) and fails to update the checksum.
> > >
> > > trivia:
> > >
> > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> > >
> > > []
> > > > @@ -574,23 +575,43 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
> > >
> > > []
> > > > + if (cotsworks) {
> > > > + dev_warn(sfp->dev,
> > > > + "EEPROM base structure checksum failure (0x%02x != 0x%02x)\n",
> > > > + check, id.base.cc_base);
> > > > + } else {
> > > > + dev_err(sfp->dev,
> > > > + "EEPROM base structure checksum failure: 0x%02x != 0x%02x\n",
> > >
> > > It'd be better to move this above the if and
> > > use only a single format string instead of
> > > using 2 slightly different formats.
> >
> > No. I think you've missed the fact that one is a _warning_ the other is
> > an _error_ and they are emitted at the appropriate severity. It's not
> > just that the format strings are slightly different.
>
> Right. Still nicer to use the same formats.
I'll stick a "Warning:" and "Error:" tag before them if you really
want the rest of the message to be identically formatted - otherwise,
when seeing reports from people's dmesg, there will be nothing to
indicate which message was printed.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 16:43 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <1074829260.1986.1522244964935.JavaMail.zimbra@efficios.com>
On 3/28/18 6:49 AM, Mathieu Desnoyers wrote:
> ----- On Mar 27, 2018, at 10:11 PM, Alexei Starovoitov ast@fb.com wrote:
>
>> From: Alexei Starovoitov <ast@kernel.org>
>>
>> compute number of arguments passed into tracepoint
>> at compile time and store it as part of 'struct tracepoint'.
>> The number is necessary to check safety of bpf program access that
>> is coming in subsequent patch.
>
>
> Hi Alexei,
>
> Given that only eBPF needs this parameter count, we can move
> it to the struct bpf_raw_event_map newly introduced by Steven,
> right ? This would reduce bloat of struct tracepoint. For instance,
> we don't need to keep this count around when eBPF is configured
> out.
Makes sense. That is indeed cleaner. Will respin.
What I don't like though is 'bloat' argument.
'u32 num_args' padded to 8-byte takes exactly the same amount
of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
The number of these structures is the same as well
and chances that tracepoints are on while bpf is off are slim.
More so few emails ago you said:
"I'm perfectly fine with adding the "num_args" stuff. I think it's
really useful. It's only the for_each_kernel_tracepoint change for
which I'm trying to understand the rationale."
I'm guessing now you found out that num_args is not useful to lttng
and it bloats its data structures?
It's ok to change an opinion and I'm completely fine using that as
a real reason.
Will repsin, as I said. No problem.
^ permalink raw reply
* Re: [PATCH 07/30] aio: add delayed cancel support
From: Al Viro @ 2018-03-28 16:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180328072926.17131-8-hch@lst.de>
On Wed, Mar 28, 2018 at 09:29:03AM +0200, Christoph Hellwig wrote:
> static void aio_fsync_work(struct work_struct *work)
> {
> struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
> + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, fsync);
> + struct file *file = req->file;
> int ret;
>
> ret = vfs_fsync(req->file, req->datasync);
> - fput(req->file);
> - aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
> + if (aio_complete(iocb, ret, 0, 0))
> + fput(file);
IDGI.
1) can aio_complete() ever return false here?
2) do we ever have aio_kiocb that would not have an associated
struct file * that needs to be dropped on successful aio_complete()? AFAICS,
rw, fsync and poll variants all have one, and I'm not sure what kind of
async IO *could* be done without an opened file.
^ permalink raw reply
* Re: 4.14.29 - tcp_push() - null skb's cb dereference
From: Eric Dumazet @ 2018-03-28 16:33 UTC (permalink / raw)
To: David Miller; +Cc: krzysztof.blaszkowski, netdev, gregkh
In-Reply-To: <20180328.103818.621024177280113028.davem@davemloft.net>
On 03/28/2018 07:38 AM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 28 Mar 2018 06:38:21 -0700
>
>> https://patchwork.ozlabs.org/patch/886324/
>
> I have this in my current -stable submission set, and I'm working
> actively on this right now.
Thanks a lot David.
^ 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