* confusing comment, explanation of @IFF_RUNNING in if.h
From: Robert P. J. Day @ 2018-08-26 8:13 UTC (permalink / raw)
To: Linux kernel netdev mailing list
more annoying pedantry ... from include/uapi/linux/if.h:
* @IFF_RUNNING: interface RFC2863 OPER_UP. Volatile.
however, both the code in net/core/dev.c:
/**
* netif_oper_up - test if device is operational
* @dev: network device
*
* Check if carrier is operational
*/
static inline bool netif_oper_up(const struct net_device *dev)
{
return (dev->operstate == IF_OPER_UP ||
dev->operstate == IF_OPER_UNKNOWN /* backward compat */);
}
and the explanation in operstates.txt:
ifinfomsg::if_flags & IFF_RUNNING:
Interface is in RFC2863 operational state UP or UNKNOWN.
suggests IFF_RUNNING represents *either* of the operational states UP
or UNKNOWN, not just UP as the comment in if.h claims. is this
misleading? or is this a deliberate explanation somehow taking into
account that the UNKNOWN state is for backward compatibility (whatever
that means)?
i ask since, in my testing, when the interface should have been up,
the attribute file "operstate" for that interface showed "unknown",
and i wondered how worried i should be about that.
rday
--
========================================================================
Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca/dokuwiki
Twitter: http://twitter.com/rpjday
LinkedIn: http://ca.linkedin.com/in/rpjday
========================================================================
^ permalink raw reply
* want to clarify understanding of IFF_{UP,RUNNING} and "volatile"
From: Robert P. J. Day @ 2018-08-26 7:39 UTC (permalink / raw)
To: Linux kernel netdev mailing list
if you can tolerate another question on the topic, based on an
earlier post by stephen hemminger, i wrote a userspace program that
opened a netlink socket to track the status changes for an interface,
and i just want to clarify how the up/running status of an interface
is determined, as i think i'm finally getting the hang of this.
as stephen mentioned, ifconfig gets its info (ultimately) from
dev_get_flags() in net/core/dev.c:
unsigned int dev_get_flags(const struct net_device *dev)
{
unsigned int flags;
flags = (dev->flags & ~(IFF_PROMISC |
IFF_ALLMULTI |
IFF_RUNNING |
IFF_LOWER_UP |
IFF_DORMANT)) |
(dev->gflags & (IFF_PROMISC |
IFF_ALLMULTI));
if (netif_running(dev)) {
if (netif_oper_up(dev))
flags |= IFF_RUNNING;
if (netif_carrier_ok(dev))
flags |= IFF_LOWER_UP;
if (netif_dormant(dev))
flags |= IFF_DORMANT;
}
return flags;
}
i don't care (yet) about IFF_{PROMISC,ALLMULTI} so i'll ignore those.
so, as i read it, whether or not the interface is *administratively*
up is read directly from the IFF_UP bit in dev->flags. so far, so
good. on the other hand, all of IFF_{RUNNING,LOWER_UP,DORMANT} are not
read from dev->flags, but instead are "volatile" and are determined
via those function calls.
so i'm guessing that shows the difference between the
administrative and the operational states of an interface -- the
administrative state of "up" is something selected by, say, ifconfig,
and can therefore be stored in the flags field, while the operational
state (running) is volatile and must be determined at each query.
this suggests that volatile flags such as running or dormant have no
value if read from dev->flags -- the code above doesn't even look at
them in that location and instead invokes the respective functions. do
i have that about right, before i crawl further down this rabbit hole?
it's all starting to make sense.
rday
--
========================================================================
Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca/dokuwiki
Twitter: http://twitter.com/rpjday
LinkedIn: http://ca.linkedin.com/in/rpjday
========================================================================
^ permalink raw reply
* Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
From: Kees Cook @ 2018-08-26 6:19 UTC (permalink / raw)
To: Al Viro
Cc: LKML, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Network Development
In-Reply-To: <20180826061534.GT6515@ZenIV.linux.org.uk>
On Sat, Aug 25, 2018 at 11:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Aug 25, 2018 at 10:58:01PM -0700, Kees Cook wrote:
>> Via u32_change(), TCA_U32_SEL has an unspecified type in the netlink
>> policy, so max length isn't enforced, only minimum. This means nkeys
>> (from userspace) was being trusted without checking the actual size of
>> nla_len(), which could lead to a memory over-read, and ultimately an
>> exposure via a call to u32_dump(). Reachability is CAP_NET_ADMIN within
>> a namespace.
>>
>> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>> Cc: Cong Wang <xiyou.wangcong@gmail.com>
>> Cc: Jiri Pirko <jiri@resnulli.us>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> This should go through -stable please, but I have left off the "Cc:
>> stable" as per netdev patch policy. Note that use of struct_size()
>> will need manual expansion in backports, such as:
>> sel_size = sizeof(*s) + sizeof(*s->keys) * s->nkeys;
>
> Saner approach would be sel_size = offsetof(struct tc_u32_sel, keys[s->nkeys])...
Either is fine by me.
>> + sel_size = struct_size(s, keys, s->nkeys);
>> + if (nla_len(tb[TCA_U32_SEL]) < sel_size) {
>> + err = -EINVAL;
>> + goto erridr;
>> + }
>>
>> - n = kzalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key), GFP_KERNEL);
>> + n = kzalloc(offsetof(typeof(*n), sel) + sel_size, GFP_KERNEL);
>
> ITYM
> n = kzalloc(offsetof(struct tc_u_common, sel.keys[s->nkeys]), GFP_KERNEL);
I prefer to reuse sel_size and keep typeof() to keep things tied to
"n" more directly. *shrug*
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
From: Al Viro @ 2018-08-26 6:15 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
David S. Miller, netdev
In-Reply-To: <20180826055801.GA42063@beast>
On Sat, Aug 25, 2018 at 10:58:01PM -0700, Kees Cook wrote:
> Via u32_change(), TCA_U32_SEL has an unspecified type in the netlink
> policy, so max length isn't enforced, only minimum. This means nkeys
> (from userspace) was being trusted without checking the actual size of
> nla_len(), which could lead to a memory over-read, and ultimately an
> exposure via a call to u32_dump(). Reachability is CAP_NET_ADMIN within
> a namespace.
>
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This should go through -stable please, but I have left off the "Cc:
> stable" as per netdev patch policy. Note that use of struct_size()
> will need manual expansion in backports, such as:
> sel_size = sizeof(*s) + sizeof(*s->keys) * s->nkeys;
Saner approach would be sel_size = offsetof(struct tc_u32_sel, keys[s->nkeys])...
> + sel_size = struct_size(s, keys, s->nkeys);
> + if (nla_len(tb[TCA_U32_SEL]) < sel_size) {
> + err = -EINVAL;
> + goto erridr;
> + }
>
> - n = kzalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key), GFP_KERNEL);
> + n = kzalloc(offsetof(typeof(*n), sel) + sel_size, GFP_KERNEL);
ITYM
n = kzalloc(offsetof(struct tc_u_common, sel.keys[s->nkeys]), GFP_KERNEL);
^ permalink raw reply
* [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
From: Kees Cook @ 2018-08-26 5:58 UTC (permalink / raw)
To: linux-kernel
Cc: Al Viro, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
netdev
Via u32_change(), TCA_U32_SEL has an unspecified type in the netlink
policy, so max length isn't enforced, only minimum. This means nkeys
(from userspace) was being trusted without checking the actual size of
nla_len(), which could lead to a memory over-read, and ultimately an
exposure via a call to u32_dump(). Reachability is CAP_NET_ADMIN within
a namespace.
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
This should go through -stable please, but I have left off the "Cc:
stable" as per netdev patch policy. Note that use of struct_size()
will need manual expansion in backports, such as:
sel_size = sizeof(*s) + sizeof(*s->keys) * s->nkeys;
---
net/sched/cls_u32.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index d5d2a6dc3921..f218ccf1e2d9 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -914,6 +914,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
struct nlattr *opt = tca[TCA_OPTIONS];
struct nlattr *tb[TCA_U32_MAX + 1];
u32 htid, flags = 0;
+ size_t sel_size;
int err;
#ifdef CONFIG_CLS_U32_PERF
size_t size;
@@ -1076,8 +1077,13 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
}
s = nla_data(tb[TCA_U32_SEL]);
+ sel_size = struct_size(s, keys, s->nkeys);
+ if (nla_len(tb[TCA_U32_SEL]) < sel_size) {
+ err = -EINVAL;
+ goto erridr;
+ }
- n = kzalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key), GFP_KERNEL);
+ n = kzalloc(offsetof(typeof(*n), sel) + sel_size, GFP_KERNEL);
if (n == NULL) {
err = -ENOBUFS;
goto erridr;
@@ -1092,7 +1098,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
}
#endif
- memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
+ memcpy(&n->sel, s, sel_size);
RCU_INIT_POINTER(n->ht_up, ht);
n->handle = handle;
n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
--
2.17.1
--
Kees Cook
Pixel Security
^ permalink raw reply related
* Re: [PATCH net 1/1] qlge: Fix netdev features configuration.
From: David Miller @ 2018-08-26 0:38 UTC (permalink / raw)
To: manish.chopra; +Cc: netdev, Dept-GELinuxNICDev, bpoirier
In-Reply-To: <20180823202052.10329-1-manish.chopra@cavium.com>
From: Manish Chopra <manish.chopra@cavium.com>
Date: Thu, 23 Aug 2018 13:20:52 -0700
> qlge_fix_features() is not supposed to modify hardware or
> driver state, rather it is supposed to only fix requested
> fetures bits. Currently qlge_fix_features() also goes for
> interface down and up unnecessarily if there is not even
> any change in features set.
>
> This patch changes/fixes following -
>
> 1) Move reload of interface or device re-config from
> qlge_fix_features() to qlge_set_features().
> 2) Reload of interface in qlge_set_features() only if
> relevant feature bit (NETIF_F_HW_VLAN_CTAG_RX) is changed.
> 3) Get rid of qlge_fix_features() since driver is not really
> required to fix any features bit.
>
> Signed-off-by: Manish <manish.chopra@cavium.com>
> Reviewed-by: Benjamin Poirier <bpoirier@suse.com>
Applied and queued up for -stable.
Please provide a proper Fixes: tag next time.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] net: macb: do not disable MDIO bus at open/close time
From: David Miller @ 2018-08-26 0:35 UTC (permalink / raw)
To: anssi.hannula; +Cc: netdev, nicolas.ferre, Claudiu.Beznea, andrew
In-Reply-To: <20180823074522.5663-1-anssi.hannula@bitwise.fi>
From: Anssi Hannula <anssi.hannula@bitwise.fi>
Date: Thu, 23 Aug 2018 10:45:22 +0300
> macb_reset_hw() is called from macb_close() and indirectly from
> macb_open(). macb_reset_hw() zeroes the NCR register, including the MPE
> (Management Port Enable) bit.
>
> This will prevent accessing any other PHYs for other Ethernet MACs on
> the MDIO bus, which remains registered at macb_reset_hw() time, until
> macb_init_hw() is called from macb_open() which sets the MPE bit again.
>
> I.e. currently the MDIO bus has a short disruption at open time and is
> disabled at close time until the interface is opened again.
>
> Fix that by only touching the RE and TE bits when enabling and disabling
> RX/TX.
>
> v2: Make macb_init_hw() NCR write a single statement.
>
> Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH v3 net 1/1] net: macb: Fix regression breaking non-MDIO fixed-link PHYs
From: David Miller @ 2018-08-26 0:31 UTC (permalink / raw)
To: a.fatoum
Cc: andrew, nicolas.ferre, kernel, netdev, mdf, brad.mouring,
f.fainelli
In-Reply-To: <20180821153548.11223-2-a.fatoum@pengutronix.de>
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
Date: Tue, 21 Aug 2018 17:35:48 +0200
> commit 739de9a1563a ("net: macb: Reorganize macb_mii bringup") broke
> initializing macb on the EVB-KSZ9477 eval board.
> There, of_mdiobus_register was called even for the fixed-link representing
> the RGMII-link to the switch with the result that the driver attempts to
> enumerate PHYs on a non-existent MDIO bus:
>
> libphy: MACB_mii_bus: probed
> mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
> [snip]
> mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
>
> The "MDIO" bus registration succeeds regardless, having claimed the reset GPIO,
> and calling of_phy_register_fixed_link later on fails because it tries
> to claim the same GPIO:
>
> macb f0028000.ethernet: broken fixed-link specification
>
> Fix this by registering the fixed-link before calling mdiobus_register.
>
> Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net] mlxsw: spectrum_switchdev: Do not leak RIFs when removing bridge
From: David Miller @ 2018-08-25 23:29 UTC (permalink / raw)
To: idosch; +Cc: netdev, jiri, petrm, alexpe, mlxsw
In-Reply-To: <20180824124135.5203-1-idosch@mellanox.com>
From: Ido Schimmel <idosch@mellanox.com>
Date: Fri, 24 Aug 2018 15:41:35 +0300
> When a bridge device is removed, the VLANs are flushed from each
> configured port. This causes the ports to decrement the reference count
> on the associated FIDs (filtering identifier). If the reference count of
> a FID is 1 and it has a RIF (router interface), then this RIF is
> destroyed.
>
> However, if no port is member in the VLAN for which a RIF exists, then
> the RIF will continue to exist after the removal of the bridge. To
> reproduce:
>
> # ip link add name br0 type bridge vlan_filtering 1
> # ip link set dev swp1 master br0
> # ip link add link br0 name br0.10 type vlan id 10
> # ip address add 192.0.2.0/24 dev br0.10
> # ip link del dev br0
>
> The RIF associated with br0.10 continues to exist.
>
> Fix this by iterating over all the bridge device uppers when it is
> destroyed and take care of destroying their RIFs.
>
> Fixes: 99f44bb3527b ("mlxsw: spectrum: Enable L3 interfaces on top of bridge devices")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reviewed-by: Petr Machata <petrm@mellanox.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH] qed: fix spelling mistake "comparsion" -> "comparison"
From: David Miller @ 2018-08-26 0:41 UTC (permalink / raw)
To: colin.king
Cc: Ariel.Elior, everest-linux-l2, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20180824111830.8657-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Fri, 24 Aug 2018 12:18:30 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> Trivial fix to spelling mistake in DP_ERR error message
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied, thanks Colin.
^ permalink raw reply
* Re: [PATCH net] vhost: correctly check the iova range when waking virtqueue
From: David Miller @ 2018-08-26 0:40 UTC (permalink / raw)
To: jasowang; +Cc: mst, kvm, virtualization, netdev, linux-kernel, peterx
In-Reply-To: <20180824085313.21798-1-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Fri, 24 Aug 2018 16:53:13 +0800
> We don't wakeup the virtqueue if the first byte of pending iova range
> is the last byte of the range we just got updated. This will lead a
> virtqueue to wait for IOTLB updating forever. Fixing by correct the
> check and wake up the virtqueue in this case.
>
> Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> The patch is needed for -stable.
Applied and queued up for -stable, thanks Jason.
^ permalink raw reply
* Re: [PATCH net] tipc: fix the big/little endian issue in tipc_dest
From: David Miller @ 2018-08-26 0:36 UTC (permalink / raw)
To: Haiqing.Bai; +Cc: netdev, jon.maloy, ying.xue, zhenbo.gao, linux-kernel
In-Reply-To: <1535014158-18578-1-git-send-email-Haiqing.Bai@windriver.com>
From: Haiqing Bai <Haiqing.Bai@windriver.com>
Date: Thu, 23 Aug 2018 16:49:18 +0800
> Fixes: d06b2fa34f18 ("tipc: improve destination linked list")
Please fix this:
[davem@localhost net]$ git describe d06b2fa34f18
fatal: Not a valid object name d06b2fa34f18
^ permalink raw reply
* Re: [PATCH v2] Revert "net: stmmac: fix build failure due to missing COMMON_CLK dependency"
From: David Miller @ 2018-08-26 0:33 UTC (permalink / raw)
To: geert
Cc: peppe.cavallaro, alexandre.torgue, joabreu, arnd, netdev,
linux-kernel
In-Reply-To: <20180822132723.30047-1-geert@linux-m68k.org>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Wed, 22 Aug 2018 15:27:23 +0200
> This reverts commit bde4975310eb1982bd0bbff673989052d92fd481.
>
> All legacy clock implementations now implement clk_set_rate() (Some
> implementations may be dummies, though).
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Arnd Bergmann <arnd@arnd.de>
> Acked-by: Jose Abreu <joabreu@synopsys.com>
> ---
> Marked "RFC", as this depends on "m68k: coldfire: Normalize clk API" and
> "MIPS: AR7: Normalize clk API".
>
> v2:
> - Add Acked-by,
> - Dropped RFC state, as all dependencies are now upstream:
> - commit eec85fa9d98a889e ("m68k: coldfire: Normalize clk API"),
> - commit 6b5939d2e528525a ("MIPS: AR7: Normalize clk API").
Applied, thank you.
^ permalink raw reply
* Re: [PATCH 1/1] net/rds: Use rdma_read_gids to get connection SGID/DGID in IPv6
From: santosh.shilimkar @ 2018-08-25 19:24 UTC (permalink / raw)
To: Zhu Yanjun, davem, netdev, linux-rdma, rds-devel
In-Reply-To: <20180825071905.2749-1-yanjun.zhu@oracle.com>
On 8/25/18 12:19 AM, Zhu Yanjun wrote:
> In IPv4, the newly introduced rdma_read_gids is used to read the SGID/DGID
> for the connection which returns GID correctly for RoCE transport as well.
>
> In IPv6, rdma_read_gids is also used. The following are why rdma_read_gids
> is introduced.
>
> rdma_addr_get_dgid() for RoCE for client side connections returns MAC
> address, instead of DGID.
> rdma_addr_get_sgid() for RoCE doesn't return correct SGID for IPv6 and
> when more than one IP address is assigned to the netdevice.
>
> So the transport agnostic rdma_read_gids() API is provided by rdma_cm
> module.
>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> ---
Looks ok.
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
^ permalink raw reply
* Re: [PATCH NET 3/3] net: hns: add configuration constraints for 1000M half
From: Andrew Lunn @ 2018-08-25 18:07 UTC (permalink / raw)
To: lipeng (Y)
Cc: davem, netdev, linux-kernel, linuxarm, yisen.zhuang, salil.mehta
In-Reply-To: <7c40c758-2ef2-665d-f429-0cc4e63a7a38@huawei.com>
> This patch is a theoretical protect, and the problem does not really
> happen.
>
> I think you really get the point, do you think we need this patch?
I think it is not needed.
And if it was needed, it would indicate there is a bug somewhere else.
Andrew
^ permalink raw reply
* Re: [PATCH v2 02/17] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-08-25 16:40 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-kernel, netdev, davem, Andy Lutomirski, Greg KH,
Samuel Neves, Jean-Philippe Aumasson, linux-crypto
In-Reply-To: <20180825062951.GC726@sol.localdomain>
Hey Eric,
On Fri, Aug 24, 2018 at 11:29:52PM -0700, Eric Biggers wrote:
> I thought you were going to wrap lines at 80 characters? It's hard to read the
> extremely long lines, and they encourage deep nesting.
I somehow noted this for the WireGuard side of things but assumed I
didn't need to do so for Zinc. Hah, such wishful thinking. I'll have
this wrapped at 80 for v3.
> As I said before, I still think you need to switch the crypto API ChaCha20 and
> Poly1305 over to use the new implementations. It's not okay to have two
> completely different sets of ChaCha20 and Poly1305 implementations just because
> you want a different API, so you might as well get started on it... The thing
> is that before you try it, it's not clear what problems will come up that
> require changes to the design. So, this really ought to be addressed up-front.
It was my hope that this entire series could enter the tree through
Dave's net-next, and that I wouldn't have to touch anything in crypto/ or
touch any of Herbert's stuff at all in anyway. However, if you want, I
can start to play with that in another branch for a separate patchset,
and of course I'd really value your feedback on that and on doing it
right.
> It's also not clearly explained whether/why/how the new ChaCha20 and Poly1305
> implementations are better than the existing ones. The patch adding the ARM and
> ARM64 ChaCha, for example, just says who wrote them, with no mention of why the
> particular implementations were chosen.
I can expand on that, sure. One primary advantage that I do touch on on
the big introductory commit message, though, is that sharing code means
that we benefit from the eyeballs and fuzzing hours spent on OpenSSL,
and I think this general advantage is extremely compelling.
> You've also documented a lot of stuff in commit messages which will be lost as
> it isn't being added to the source itself. There are various examples of this,
> such as information about where the various implementations came from, what you
> changed, why a particular implementation isn't used on Skylake or whatever, etc.
> Can you please make sure that any important information is in comments, e.g. at
> the top of the files?
Good idea. I'll do that for v3.
> I still think the "Zinc" name is confusing and misleading, and people are going
> to forget that it means "crypto". lib/crypto/ would be more intuitive.
> But I don't care *that* much myself, and you should see what others think...
I'd like to keep it. It also enables a natural path for a corresponding
userspace library that shares all of the same code, and one that I think
will be compelling to attract academics and developers to contributing
to these efforts. Plus, can't I name what I made?
> It seems you still don't explicitly clarify anywhere in the source itself that
> the copyright holders of the code from OpenSSL have relicensed it under GPLv2.
> I only see a GPLv2 license slapped on the files, yet no such license is presence
> in the OpenSSL originals, at least in the one I checked.
The SPDX headers for those came out of a discussion on how to encode
CRYPTOGAMS into SPDX from the SPDX mailing list several months ago.
> If you did receive
> explicit permission, then you should include an explicit clarification in each
> file like the one in arch/arm/crypto/sha1-armv4-large.S.
That's a good idea.
> As Ard and I discussed recently on my patch
> "crypto: arm/poly1305 - add NEON accelerated Poly1305 implementation"
> which proposed adding the exact same poly1305-arm.S file, for all the OpenSSL
> assembly it would probably be better to include the .pl file and generate the .S
> file as part of the build process. For one, there is semantic information like
> register names in the .pl script that is lost in the .S file, thereby making the
> .S file less readable.
But on the other hand, the .S files have been modified and changed to
fit kernel constraints and conventions. They're very much no longer
merely "generated". This is most apparent with the x86_64 files, but is
present too with the ARM files. We're still, I believe, bug-for-bug
similar to the originals -- keeping with the point of going with Andy's
implementations in the first place -- and we've been able to pretty
trivially track OpenSSL changes -- but nonetheless important tweaks have
been done to make this suitable to kernel space.
> There are still some alignment bugs where integers are loaded from byte arrays
> without using the unaligned access macros, e.g. in chacha20_init(),
> hchacha20_generic(), and fe_frombytes_impl(). I found these grepping for
> le32_to_cpu. Interestingly, that last one is in "formally verified" code :-)
Thanks. I'll do another pass at these for v3.
Jason
^ permalink raw reply
* Re: [net 01/11] ixgb: use dma_zalloc_coherent instead of allocator/memset
From: Joe Perches @ 2018-08-25 15:49 UTC (permalink / raw)
To: Jeff Kirsher, davem; +Cc: YueHaibing, netdev, nhorman, sassmann, jogreene
In-Reply-To: <20180824184735.32175-2-jeffrey.t.kirsher@intel.com>
On Fri, 2018-08-24 at 11:47 -0700, Jeff Kirsher wrote:
> From: YueHaibing <yuehaibing@huawei.com>
>
> Use dma_zalloc_coherent instead of dma_alloc_coherent
> followed by memset 0.
Unrelated trivia: above this, perhaps the
size = sizeof(struct ixgb_buffer) * txdr->count;
txdr->buffer_info = vzalloc(size);
if (!txdr->buffer_info)
return -ENOMEM;
should be changed to kvzalloc to avoid using vzalloc where
possible, with the appropriate vfree/kvfree conversions.
Perhaps across all intel drivers:
$ git grep -n "\bv.alloc(" drivers/net/ethernet/intel
drivers/net/ethernet/intel/e1000/e1000_main.c:1499: txdr->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/e1000/e1000_main.c:1689: rxdr->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/e1000e/ethtool.c:705: temp_tx = vmalloc(size);
drivers/net/ethernet/intel/e1000e/ethtool.c:712: temp_rx = vmalloc(size);
drivers/net/ethernet/intel/e1000e/netdev.c:2328: tx_ring->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/e1000e/netdev.c:2363: rx_ring->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c:561: temp_ring = vmalloc(array_size(i, sizeof(struct fm10k_ring)));
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:22: tx_ring->tx_buffer = vzalloc(size);
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:90: rx_ring->rx_buffer = vzalloc(size);
drivers/net/ethernet/intel/igb/igb_ethtool.c:905: temp_ring = vmalloc(array_size(sizeof(struct igb_ring),
drivers/net/ethernet/intel/igb/igb_ethtool.c:908: temp_ring = vmalloc(array_size(sizeof(struct igb_ring),
drivers/net/ethernet/intel/igb/igb_main.c:4075: tx_ring->tx_buffer_info = vmalloc(size);
drivers/net/ethernet/intel/igb/igb_main.c:4224: rx_ring->rx_buffer_info = vmalloc(size);
drivers/net/ethernet/intel/igbvf/ethtool.c:226: temp_ring = vmalloc(sizeof(struct igbvf_ring));
drivers/net/ethernet/intel/igbvf/netdev.c:421: tx_ring->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/igbvf/netdev.c:459: rx_ring->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/ixgb/ixgb_main.c:682: txdr->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/ixgb/ixgb_main.c:765: rxdr->buffer_info = vzalloc(size);
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c:1074: temp_ring = vmalloc(array_size(i, sizeof(struct ixgbe_ring)));
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6352: tx_ring->tx_buffer_info = vmalloc(size);
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6446: rx_ring->rx_buffer_info = vmalloc(size);
drivers/net/ethernet/intel/ixgbevf/ethtool.c:285: tx_ring = vmalloc(array_size(sizeof(*tx_ring),
drivers/net/ethernet/intel/ixgbevf/ethtool.c:331: rx_ring = vmalloc(array_size(sizeof(*rx_ring),
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:3373: tx_ring->tx_buffer_info = vmalloc(size);
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:3451: rx_ring->rx_buffer_info = vmalloc(size);
^ permalink raw reply
* Re: Regression: youtube-dl hanging on download
From: David Ahern @ 2018-08-25 15:47 UTC (permalink / raw)
To: Jan Janssen; +Cc: davem, kuznet, yoshfuji, netdev
In-Reply-To: <36553198.DNrMIUSMyK@minako>
On 8/25/18 9:23 AM, Jan Janssen wrote:
> Hi,
>
> youtube-dl hangs trying to download videos from youtube. The commit below is
> my regression test result. Disabling ipv6 indeed does fix this for me.
>
Thanks for the report. Can you boot a pre-4.18 kernel and send me,
offlist, the output of 'ip -6 ro ls table all'?
^ permalink raw reply
* Regression: youtube-dl hanging on download
From: Jan Janssen @ 2018-08-25 15:23 UTC (permalink / raw)
To: dsahern; +Cc: davem, kuznet, yoshfuji, netdev
Hi,
youtube-dl hangs trying to download videos from youtube. The commit below is
my regression test result. Disabling ipv6 indeed does fix this for me.
Jan
---
d4ead6b34b67fd711639324b6465a050bcb197d4 is the first bad commit
commit d4ead6b34b67fd711639324b6465a050bcb197d4
Author: David Ahern <dsahern@gmail.com>
Date: Tue Apr 17 17:33:16 2018 -0700
net/ipv6: move metrics from dst to rt6_info
Similar to IPv4, add fib metrics to the fib struct, which at the moment
is rt6_info. Will be moved to fib6_info in a later patch. Copy metrics
into dst by reference using refcount.
To make the transition:
- add dst_metrics to rt6_info. Default to dst_default_metrics if no
metrics are passed during route add. No need for a separate pmtu
entry; it can reference the MTU slot in fib6_metrics
- ip6_convert_metrics allocates memory in the FIB entry and uses
ip_metrics_convert to copy from netlink attribute to metrics entry
- the convert metrics call is done in ip6_route_info_create simplifying
the route add path
+ fib6_commit_metrics and fib6_copy_metrics and the temporary
mx6_config are no longer needed
- add fib6_metric_set helper to change the value of a metric in the
fib entry since dst_metric_set can no longer be used
- cow_metrics for IPv6 can drop to dst_cow_metrics_generic
- rt6_dst_from_metrics_check is no longer needed
- rt6_fill_node needs the FIB entry and dst as separate arguments to
keep compatibility with existing output. Current dst address is
renamed to dest.
(to be consistent with IPv4 rt6_fill_node really should be split
into 2 functions similar to fib_dump_info and rt_fill_info)
- rt6_fill_node no longer needs the temporary metrics variable
Signed-off-by: David Ahern <dsahern@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
:040000 040000 1256a75be6bb5b9f3ef5b38ff75cfd10aba1a856
d6cc99cbefe99bb3a5c1b73b6dcccd8ec0816ec5 M include
:040000 040000 36a3fa59bea55e68c88a4fe2d1ac653fa8a50705
cdbd8a8aa4d8bc2e8abf6bdf18b30388a957183c M net
^ permalink raw reply
* TRADING ACCOUNT
From: KELLY ALAN @ 2018-08-25 14:32 UTC (permalink / raw)
To: kellyalan
Dear sir ,
I KELLY ALAN purchasing and sales manager of CFM INTERNATIONAL .Our
Company specialised in Supplying computer hardware and Electronic .We
want to extend our supplier list because of concurrency in prices on the
international market. We are seeking a supplier with whom we can to have
partnered long-term in order to have competitive prices . we are
interested to buy the products you sell and want to place an order with
you in big quantities.
Can you give us payment facilities ( 14 , 30 or 60 days payment terms )
?
What is the procedure for our account opening and credit line
application ?
Cordially
KELLY ALAN
CFM INTERNATIONAL
2 BOULEVARD DU GAL MARTIAL VALIN
75015 PARIS
REG N° 302 527 700
VAT N° FR90 302527700
TEL +33171025367
FAX +33177759149
https://www.cfmaeroengines.com
^ permalink raw reply
* Re: [PATCH v2 02/17] zinc: introduce minimal cryptography library
From: Andrew Lunn @ 2018-08-25 17:26 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Eric Biggers, linux-kernel, netdev, davem, Andy Lutomirski,
Greg KH, Samuel Neves, Jean-Philippe Aumasson, linux-crypto
In-Reply-To: <20180825164005.GA7748@zx2c4.com>
On Sat, Aug 25, 2018 at 10:40:06AM -0600, Jason A. Donenfeld wrote:
> Hey Eric,
>
> On Fri, Aug 24, 2018 at 11:29:52PM -0700, Eric Biggers wrote:
> > I thought you were going to wrap lines at 80 characters? It's hard to read the
> > extremely long lines, and they encourage deep nesting.
>
> I somehow noted this for the WireGuard side of things but assumed I
> didn't need to do so for Zinc. Hah, such wishful thinking. I'll have
> this wrapped at 80 for v3.
Hi Jason
The coding style document applies to all code in the tree. Plus some
out of tree stuff, like iproute2 if, you need user space patches for
that.
I've been running checkpatch on just the network parts. But i expect
at some point, somebody will run it on the crypto stuff as well, and
ask you to fix up some of the errors and warning.
There is also a general trend that you won't get in detail human
reviews until the automated review tools indicate the code is O.K.,
Andrew
^ permalink raw reply
* Re: [PATCH v2 02/17] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-08-25 17:17 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: LKML, Netdev, David Miller, Andrew Lutomirski, Greg Kroah-Hartman,
Samuel Neves, Jean-Philippe Aumasson, Linux Crypto Mailing List
In-Reply-To: <20180825170629.GA8971@zx2c4.com>
Pressed send too fast.
On Sat, Aug 25, 2018 at 11:06 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> - https://www.wireguard.com/papers/wireguard-formal-verification.pdf
> - https://www.wireguard.com/papers/dowling-paterson-computational-2018.pdf
> - https://www.wireguard.com/formal-verification/
- https://www.wireguard.com/papers/lipp-computational-2018.pdf
- https://eprint.iacr.org/2018/766.pdf
^ permalink raw reply
* Re: [PATCH] chtls: fix null dereference chtls_free_uld()
From: Herbert Xu @ 2018-08-25 13:27 UTC (permalink / raw)
To: Ganesh Goudar; +Cc: linux-crypto, netdev, davem, indranil, dt, Atul Gupta
In-Reply-To: <1533905861-27339-1-git-send-email-ganeshgr@chelsio.com>
On Fri, Aug 10, 2018 at 06:27:41PM +0530, Ganesh Goudar wrote:
> call chtls_free_uld() only for the initialized cdev,
> this fixes NULL dereference in chtls_free_uld()
>
> Signed-off-by: Ganesh Goudar <ganeshgr@chelsio.com>
> Signed-off-by: Atul Gupta <atul.gupta@chelsio.com>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v2 02/17] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-08-25 17:06 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux Kernel Mailing List, <netdev@vger.kernel.org>,
David S. Miller, Andy Lutomirski, Greg KH, Samuel Neves,
Jean-Philippe Aumasson,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE
In-Reply-To: <CAKv+Gu-u7vPkU49AzToGHyf_cWp0MiOmpD3cRUt8260QC2wj-Q@mail.gmail.com>
Hello Ard,
On Sat, Aug 25, 2018 at 11:17:42AM +0100, Ard Biesheuvel wrote:
> Do we really need a new crypto API for WireGuard? Surely, you yourself
> are not affected by these concerns, and I don't anticipate an
> explosion of new crypto use cases in the kernel that would justify
> maintaining two parallel crypto stacks.
Yes, we really do. And the new API won't be useful for WireGuard but
also for the majority of cryptography users within the kernel, which
will gradually be ported to use the new API.
> Also, I take it this means that WireGuard will only work with your
> routines? We don't usually impose that kind of policy in the kernel:
> on my arm64 systems, AES/GCM runs at 2.3 cycles per byte, which is a
> good enough reason to prefer it over a ChaCha20/Poly1305 based AEAD,
> even if your code is faster than the current code (which is debatable,
> as you know)
WireGuard is a protocol that specifies ChaPoly, and I am proposing an
implementation of that protocol. The protocol explicitly does not
specify other ciphers. Please read the whitepaper for extensive
discussion and feel free to come on over to wireguard@lists.zx2c4.com if
you really want to hardcore bikeshed on protocol particulars. The short
answer, however, is: no WireGuard won't be adding cipher negotiation,
and no, this isn't considered a good feature, and no, you won't have
any luck changing that. I am certain, though, that pushing this point
will be a terrific means of steering this implementation thread far off
course, as various folks with their various opinions feel compelled to
jump in about their thoughts on cipher negotiation. Yawn.
Besides, your claim about "impos[ing] that kind of policy in the kernel"
is bogus: with the exception of IPsec, dm-crypt, and AF_ALG, basically
all users of cryptography I can find are using a very particular set of
ciphers. For example, Bluetooth uses a particular elliptic curve that
has been specified by the Bluetooth people, and the kernel therefore
implements a driver that does computations over that elliptic curve.
Nobody on LKML is clamoring for that driver to also support Curve448 or
something, since that's not what Bluetooth specifies or how Bluetooth
works. So too, that's not what WireGuard specifies or how WireGuard
works.
> It also means that users of the crypto API will not benefit from your
> better code. I'm sure you agree that it is a bad idea to force those
> same crypto-impaired programmers to port their code to a new API.
As mentioned, I think we'll certainly be gradually porting existing
crypto API users over to the new API where appropriate. As always, this
is a gradual thing, and won't ever come all at once in the first
patchset, but I'm pretty confident we can get there somewhat quickly,
especially as new primitives become available in the new API.
> I also suggested that we work with Andy Polyakov (as I have in the
> past) to make the changes required for the kernel in the OpenSSL
> upstream. That way, we can take the .pl files unmodified, and benefit
> from the maintenance and review upstream. Dumping 10,000s of lines of
> generated assembler in the kernel tree like that is really
> unacceptable IMO.
I'm happy to do that, and indeed I do enjoy working with Andy. However,
I don't think your characterization of the current situation is at all
accurate. Rather, those .S files have been extensively worked with and
crafted. They're far from being merely generated, and they've been
pretty heavily reviewed.
> I'm not sure what the relevance of
> formally verified implementations is, though, since it only proves
> that the code is true to the algorithm, not that the algorithm is
> secure. Or am I missing something?
You are indeed missing something.
When the code is not true to the algorithm, that is very bad.
When the code is true to the algorithm, that is very good.
Formally verified implementations intend to increase our confidence
in the latter.
Meanwhile on the algorithm side of thing, the WireGuard protocol has a
number of proofs that the protocol is secure:
- https://www.wireguard.com/papers/wireguard-formal-verification.pdf
- https://www.wireguard.com/papers/dowling-paterson-computational-2018.pdf
- https://www.wireguard.com/formal-verification/
Before you get too excited though, keep in mind that this is a proof of
the protocol, and not of each primitive, like, say, "ChaCha20" or
"Curve25519." However, academic literature is full of all sorts of
curious and fascinating security analyses of the various primitives if
this is something you're interested in.
Jason
^ permalink raw reply
* Re: broken behaviour of TC filter delete
From: Jiri Pirko @ 2018-08-25 13:02 UTC (permalink / raw)
To: Cong Wang
Cc: Roman Mashak, Linux Kernel Network Developers, Jiri Pirko,
Jamal Hadi Salim
In-Reply-To: <CAM_iQpXDG4YdiUrqFhn7fxsbTBMBdR4x-ZQQ3U5E_xSPf-PANQ@mail.gmail.com>
Fri, Aug 24, 2018 at 08:11:07PM CEST, xiyou.wangcong@gmail.com wrote:
>On Fri, Aug 24, 2018 at 9:21 AM Roman Mashak <mrv@mojatatu.com> wrote:
>>
>> So _before_ commit f71e0ca4db187af7c44987e9d21e9042c3046070 step 6 would
>> return -ENOENT with "Error: Filter with specified priority/protocol not
>> found." and _after_ the commit it returns -EINVAL (Error: Cannot find
>> specified filter chain.)
>>
>> ENOENT seems to be more logical to return when there's no more filter to delete.
>
>Yeah, at least we should keep ENOENT for compatibility.
>
>The bug here is chain 0 is gone after the last filter is gone,
>so when you delete the filter again, it treats it as you specify
>chain 0 which does not exist, so it hits EINVAL before ENOENT.
I understand. My concern is about consistency with other chains. Perhaps
-ENOENT for all chains in this case would be doable. What do you think?
>
>I am not sure how to fix this properly.
^ 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