Re: [PATCH net-next 2/2] net: socket: change MSG_CMSG_COMPAT to BIT(21)
On Sun, Mar 21, 2021 at 08:39:29PM +0800, menglong8.d...@gmail.com wrote:
>
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index d5ebfe30d96b..317b2933f499 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -312,17 +312,18 @@ struct ucred {
>* plain text and require encryption
>*/
>
> +#if defined(CONFIG_COMPAT)
> +#define MSG_CMSG_COMPAT BIT(21) /* This message needs 32 bit
> fixups */
> +#else
> +#define MSG_CMSG_COMPAT 0 /* We never have 32 bit fixups
> */
> +#endif
> +
> #define MSG_ZEROCOPY BIT(26) /* Use user data in kernel path */
> #define MSG_FASTOPEN BIT(29) /* Send data in TCP SYN */
> #define MSG_CMSG_CLOEXEC BIT(30) /* Set close_on_exec for file
>* descriptor received through
>* SCM_RIGHTS
>*/
> -#if defined(CONFIG_COMPAT)
> -#define MSG_CMSG_COMPAT BIT(31) /* This message needs 32 bit
> fixups */
> -#else
> -#define MSG_CMSG_COMPAT 0 /* We never have 32 bit fixups
> */
> -#endif
Shouldn't you add some comment here to stop people from trying to
use BIT(31) in the future?
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net] macvlan: macvlan_count_rx() needs to be aware of preemption
On Wed, Mar 10, 2021 at 01:56:36AM -0800, Eric Dumazet wrote:
> From: Eric Dumazet
>
> macvlan_count_rx() can be called from process context, it is thus
> necessary to disable preemption before calling u64_stats_update_begin()
>
> syzbot was able to spot this on 32bit arch:
>
> WARNING: CPU: 1 PID: 4632 at include/linux/seqlock.h:271 __seqprop_assert
> include/linux/seqlock.h:271 [inline]
> WARNING: CPU: 1 PID: 4632 at include/linux/seqlock.h:271
> __seqprop_assert.constprop.0+0xf0/0x11c include/linux/seqlock.h:269
> Modules linked in:
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 4632 Comm: kworker/1:3 Not tainted 5.12.0-rc2-syzkaller #0
> Hardware name: ARM-Versatile Express
> Workqueue: events macvlan_process_broadcast
> Backtrace:
> [<82740468>] (dump_backtrace) from [<827406dc>] (show_stack+0x18/0x1c
> arch/arm/kernel/traps.c:252)
> r7:0080 r6:6093 r5: r4:8422a3c4
> [<827406c4>] (show_stack) from [<82751b58>] (__dump_stack lib/dump_stack.c:79
> [inline])
> [<827406c4>] (show_stack) from [<82751b58>] (dump_stack+0xb8/0xe8
> lib/dump_stack.c:120)
> [<82751aa0>] (dump_stack) from [<82741270>] (panic+0x130/0x378
> kernel/panic.c:231)
> r7:830209b4 r6:84069ea4 r5: r4:844350d0
> [<82741140>] (panic) from [<80244924>] (__warn+0xb0/0x164 kernel/panic.c:605)
> r3:8404ec8c r2: r1: r0:830209b4
> r7:010f
> [<80244874>] (__warn) from [<82741520>] (warn_slowpath_fmt+0x68/0xd4
> kernel/panic.c:628)
> r7:81363f70 r6:010f r5:83018e50 r4:
> [<827414bc>] (warn_slowpath_fmt) from [<81363f70>] (__seqprop_assert
> include/linux/seqlock.h:271 [inline])
> [<827414bc>] (warn_slowpath_fmt) from [<81363f70>]
> (__seqprop_assert.constprop.0+0xf0/0x11c include/linux/seqlock.h:269)
> r8:5a109000 r7:000f r6:a568dac0 r5:89802300 r4:0001
> [<81363e80>] (__seqprop_assert.constprop.0) from [<81364af0>]
> (u64_stats_update_begin include/linux/u64_stats_sync.h:128 [inline])
> [<81363e80>] (__seqprop_assert.constprop.0) from [<81364af0>]
> (macvlan_count_rx include/linux/if_macvlan.h:47 [inline])
> [<81363e80>] (__seqprop_assert.constprop.0) from [<81364af0>]
> (macvlan_broadcast+0x154/0x26c drivers/net/macvlan.c:291)
> r5:89802300 r4:8a927740
> [<8136499c>] (macvlan_broadcast) from [<81365020>]
> (macvlan_process_broadcast+0x258/0x2d0 drivers/net/macvlan.c:317)
> r10:81364f78 r9:8a86d000 r8:8a9c7e7c r7:8413aa5c r6: r5:
> r4:89802840
> [<81364dc8>] (macvlan_process_broadcast) from [<802696a4>]
> (process_one_work+0x2d4/0x998 kernel/workqueue.c:2275)
> r10:0008 r9:8404ec98 r8:84367a02 r7:ddfe6400 r6:ddfe2d40 r5:898dac80
> r4:8a86d43c
> [<802693d0>] (process_one_work) from [<80269dcc>] (worker_thread+0x64/0x54c
> kernel/workqueue.c:2421)
> r10:0008 r9:8a9c6000 r8:84006d00 r7:ddfe2d78 r6:898dac94 r5:ddfe2d40
> r4:898dac80
> [<80269d68>] (worker_thread) from [<80271f40>] (kthread+0x184/0x1a4
> kernel/kthread.c:292)
> r10:85247e64 r9:898dac80 r8:80269d68 r7: r6:8a9c6000 r5:89a2ee40
> r4:8a97bd00
> [<80271dbc>] (kthread) from [<80200114>] (ret_from_fork+0x14/0x20
> arch/arm/kernel/entry-common.S:158)
> Exception stack(0x8a9c7fb0 to 0x8a9c7ff8)
>
> Fixes: 412ca1550cbe ("macvlan: Move broadcasts into a work queue")
> Signed-off-by: Eric Dumazet
> Cc: Herbert Xu
> Reported-by: syzbot
> ---
> include/linux/if_macvlan.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Acked-by: Herbert Xu
Thanks!
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: Rename struct device_private to bcm_device_private
On Tue, Jan 05, 2021 at 12:02:37AM +0100, Jiri Olsa wrote: > Renaming 'struct device_private' to 'struct bcm_device_private', > because it clashes with 'struct device_private' from > 'drivers/base/base.h'. > > While it's not a functional problem, it's causing two distinct > type hierarchies in BTF data. It also breaks build with options: > CONFIG_DEBUG_INFO_BTF=y > CONFIG_CRYPTO_DEV_BCM_SPU=y > > as reported by Qais Yousef [1]. > > [1] https://lore.kernel.org/lkml/20201229151352.6hzmjvu3qh6p2qgg@e107158-lin/ > Signed-off-by: Jiri Olsa > --- > drivers/crypto/bcm/cipher.c | 2 +- > drivers/crypto/bcm/cipher.h | 4 ++-- > drivers/crypto/bcm/util.c | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Why the auxiliary cipher in gss_krb5_crypto.c?
On Fri, Dec 04, 2020 at 06:35:48PM +0100, Ard Biesheuvel wrote: > > Herbert recently made some changes for MSG_MORE support in the AF_ALG > code, which permits a skcipher encryption to be split into several > invocations of the skcipher layer without the need for this complexity > on the side of the caller. Maybe there is a way to reuse that here. > Herbert? Yes this was one of the reasons I was persuing the continuation work. It should allow us to kill the special case for CTS in the krb5 code. Hopefully I can get some time to restart work on this soon. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC][PATCH 00/18] crypto: Add generic Kerberos library
On Tue, Dec 01, 2020 at 09:12:38AM +, David Howells wrote: > > That depends on whether the caller has passed it elsewhere for some other > parallel purpose, but I think I'm going to have to go down that road and > restore it afterwards. Sure but even if you added it to the API the underlying implementataions would just have to do the same thing. Since this is particular to your use-case it's better to leave the complexity where it's needed rather than propagting it to all the crypto drivers. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC][PATCH 00/18] crypto: Add generic Kerberos library
On Tue, Dec 01, 2020 at 08:44:33AM +, David Howells wrote: > Btw, would it be feasible to make it so that an extra parameter can be added > to the cipher buffer-supplying functions, e.g.: > > skcipher_request_set_crypt(req, input, ciphertext_sg, esize, iv); > > such that we can pass in an offset into the output sg as well? Couldn't you just change the output sg to include the offset? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC][PATCH 00/18] crypto: Add generic Kerberos library
On Thu, Nov 26, 2020 at 08:19:41AM +, David Howells wrote: > > I haven't done that yet. Sorry, I should've been more explicit with what I > was after. I was wanting to find out if the nfs/nfsd people are okay with > this (and if there are any gotchas I should know about - it turns out, if I > understand it correctly, the relevant code may being being rewritten a bit > anyway). > > And from you, I was wanting to find out if you're okay with an interface of > this kind in crypto/ where the code is just used directly - or whether I'll > be required to wrap it up in the autoloading, module-handling mechanisms. I don't have any problems with it living under crypto. However, I'd like to see what the sunrpc code looks like before going one way or another. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC][PATCH 00/18] crypto: Add generic Kerberos library
On Thu, Nov 12, 2020 at 12:57:45PM +, David Howells wrote: > > Hi Herbert, Bruce, > > Here's my first cut at a generic Kerberos crypto library in the kernel so > that I can share code between rxrpc and sunrpc (and cifs?). Hi David: I can't find the bit where you are actually sharing this code with sunrpc, am I missing something? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device
On Tue, Nov 24, 2020 at 11:48:35AM +0100, Eric Dumazet wrote: > > Well, the 'increment' part was suggesting the function was adding > flags, not removing them. The idea of the increment part is that we're adding a constituent device, not that we're adding features. There have always been features which were conjunctions, i.e., they must be supported by all underlying devices for them to be enabled on the virtual device. Your use of the increment function is unusual, as you're not adding features that belong to one underlying device, but rather you're trying to enable a feature on the virtual device unconditionally. > We might ask Herbert Xu if we : > > 1) Need to comment the function, or change its name to be more descriptive. > 2) Change the behavior (as you suggested) > 3) Other choice. I think Tariq's patch is fine, although a comment should be added to netdev_add_tso_features as this use of the increment function is nonstandard. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v9,net-next,12/12] crypto: octeontx2: register with linux crypto framework
On Fri, Nov 13, 2020 at 08:44:40AM -0800, Jakub Kicinski wrote: > > SGTM, actually everything starting from patch 4 is in drivers/crypto, > so we can merge the first 3 into net-next and the rest via crypto? Yes of course. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v9,net-next,12/12] crypto: octeontx2: register with linux crypto framework
On Wed, Nov 11, 2020 at 04:10:39PM -0800, Jakub Kicinski wrote: > On Mon, 9 Nov 2020 17:39:24 +0530 Srujana Challa wrote: > > CPT offload module utilises the linux crypto framework to offload > > crypto processing. This patch registers supported algorithms by > > calling registration functions provided by the kernel crypto API. > > > > The module currently supports: > > - AES block cipher in CBC,ECB,XTS and CFB mode. > > - 3DES block cipher in CBC and ECB mode. > > - AEAD algorithms. > > authenc(hmac(sha1),cbc(aes)), > > authenc(hmac(sha256),cbc(aes)), > > authenc(hmac(sha384),cbc(aes)), > > authenc(hmac(sha512),cbc(aes)), > > authenc(hmac(sha1),ecb(cipher_null)), > > authenc(hmac(sha256),ecb(cipher_null)), > > authenc(hmac(sha384),ecb(cipher_null)), > > authenc(hmac(sha512),ecb(cipher_null)), > > rfc4106(gcm(aes)). > > Herbert, could someone who knows about crypto take a look at this, > if the intention is to merge this via net-next? This patch seems to be quite large but it is self-contained. How about waiting a release cycle and then resubmitting it to linux-crypto on its own? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH ipsec] xfrm: Pass template address family to xfrm_state_look_at
On Mon, Nov 02, 2020 at 06:32:19PM -0800, Anthony DeRossi wrote: > This fixes a regression where valid selectors are incorrectly skipped > when xfrm_state_find is called with a non-matching address family (e.g. > when using IPv6-in-IPv4 ESP in transport mode). Why are we even allowing v6-over-v4 in transport mode? Isn't that the whole point of BEET mode? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH ipsec] xfrm: Pass template address family to xfrm_state_look_at
On Mon, Nov 02, 2020 at 06:32:19PM -0800, Anthony DeRossi wrote:
> This fixes a regression where valid selectors are incorrectly skipped
> when xfrm_state_find is called with a non-matching address family (e.g.
> when using IPv6-in-IPv4 ESP in transport mode).
>
> The state's address family is matched against the template's family
> (encap_family) in xfrm_state_find before checking the selector in
> xfrm_state_look_at. The template's family should also be used for
> selector matching, otherwise valid selectors may be skipped.
>
> Fixes: e94ee171349d ("xfrm: Use correct address family in xfrm_state_find")
> Signed-off-by: Anthony DeRossi
> ---
> net/xfrm/xfrm_state.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Your patch reintroduces the same bug that my patch was trying to
fix, namely that when you do the comparison on flow you must use
the original family and not some other value.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] xfrm/compat: Remove use of kmalloc_track_caller
On Sun, Nov 01, 2020 at 02:08:45PM -0800, Alistair Delva wrote: > The __kmalloc_track_caller symbol is not exported if SLUB/SLOB are > enabled instead of SLAB, which breaks the build on such configs when > CONFIG_XFRM_USER_COMPAT=m. > > ERROR: "__kmalloc_track_caller" [net/xfrm/xfrm_compat.ko] undefined! Is this with a recent kernel? Because they should be exported: commit fd7cb5753ef49964ea9db5121c3fc9a4ec21ed8e Author: Daniel Vetter Date: Mon Mar 23 15:49:00 2020 +0100 mm/sl[uo]b: export __kmalloc_track(_node)_caller Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2] net: xfrm: fix a race condition during allocing spi
On Thu, Oct 22, 2020 at 06:01:27PM +0800, Zhuoliang Zhang wrote:
> From: zhuoliang zhang
>
> we found that the following race condition exists in
> xfrm_alloc_userspi flow:
>
> user threadstate_hash_work thread
>
> xfrm_alloc_userspi()
> __find_acq_core()
>/*alloc new xfrm_state:x*/
>xfrm_state_alloc()
>/*schedule state_hash_work thread*/
>xfrm_hash_grow_check()xfrm_hash_resize()
> xfrm_alloc_spi /*hold lock*/
> x->id.spi = htonl(spi)
> spin_lock_bh(&net->xfrm.xfrm_state_lock)
> /*waiting lock release*/ xfrm_hash_transfer()
> spin_lock_bh(&net->xfrm.xfrm_state_lock) /*add x into
> hlist:net->xfrm.state_byspi*/
>
> hlist_add_head_rcu(&x->byspi)
>
> spin_unlock_bh(&net->xfrm.xfrm_state_lock)
>
> /*add x into hlist:net->xfrm.state_byspi 2 times*/
> hlist_add_head_rcu(&x->byspi)
>
> 1. a new state x is alloced in xfrm_state_alloc() and added into the bydst
> hlist
> in __find_acq_core() on the LHS;
> 2. on the RHS, state_hash_work thread travels the old bydst and tranfers
> every xfrm_state
> (include x) into the new bydst hlist and new byspi hlist;
> 3. user thread on the LHS gets the lock and adds x into the new byspi hlist
> again.
>
> So the same xfrm_state (x) is added into the same list_hash
> (net->xfrm.state_byspi) 2 times that makes the list_hash become
> an inifite loop.
>
> To fix the race, x->id.spi = htonl(spi) in the xfrm_alloc_spi() is moved
> to the back of spin_lock_bh, sothat state_hash_work thread no longer add x
> which id.spi is zero into the hash_list.
>
> Fixes: f034b5d4efdf ("[XFRM]: Dynamic xfrm_state hash table sizing.")
> Signed-off-by: zhuoliang zhang
> ---
> net/xfrm/xfrm_state.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
Acked-by: Herbert Xu
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] net: xfrm: fix a race condition during allocing spi
On Thu, Oct 22, 2020 at 01:53:15PM +0800, zhuoliang.zhang wrote: > > there are 2 related hash lists : net->xfrm.state_bydst and > net->xfrm.state_byspi: > > 1. a new state x is alloced in xfrm_state_alloc() and added into the > bydst hlist in __find_acq_core() on the LHS; > 2. on the RHS, state_hash_work thread travels the old bydst and tranfers > every xfrm_state (include x) to the new bydst hlist and new byspi hlist; > 3. user thread on the LHS gets the lock and adds x to the new byspi > hlist again. Good catch. Please add a Fixes header. I think this was introduced with the dynamic resizing in f034b5d4efdfe0fb9e2a1ce1d95fa7914f24de49. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] net: xfrm: fix a race condition during allocing spi
On Tue, Oct 20, 2020 at 04:18:00PM +0800, Zhuoliang Zhang wrote: > From: zhuoliang zhang > > we found that the following race condition exists in > xfrm_alloc_userspi flow: > > user threadstate_hash_work thread > > xfrm_alloc_userspi() > __find_acq_core() >/*alloc new xfrm_state:x*/ >xfrm_state_alloc() >/*schedule state_hash_work thread*/ >xfrm_hash_grow_check()xfrm_hash_resize() > xfrm_alloc_spi /*hold lock*/ > x->id.spi = htonl(spi) > spin_lock_bh(&net->xfrm.xfrm_state_lock) > /*waiting lock release*/ xfrm_hash_transfer() > spin_lock_bh(&net->xfrm.xfrm_state_lock) /*add x into > hlist:net->xfrm.state_byspi*/ > > hlist_add_head_rcu(&x->byspi) > > spin_unlock_bh(&net->xfrm.xfrm_state_lock) > > /*add x into hlist:net->xfrm.state_byspi 2 times*/ > hlist_add_head_rcu(&x->byspi) > > So the same xfrm_stame (x) is added into the same list_hash > (net->xfrm.state_byspi)2 times that makes the list_hash become > a inifite loop. Your explanation makes no sense. Prior to obtaining the spin lock on the LHS, the state x hasn't been added to thte table yet. So how can the RHS be transferring it? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: gssapi, crypto and afs/rxrpc
On Fri, Oct 16, 2020 at 05:18:26PM +0100, David Howells wrote: > > If I do this, should I create a "kerberos" crypto API for the data wrapping > functions? I'm not sure that it quite matches the existing APIs because the > size of the input data will likely not match the size of the output data and > it's "one shot" as it needs to deal with a checksum. Generally it makes sense to create a Crypto API for an algorithm if there are going to be at least two implementations of it. In particular, if there is hardware acceleration available then it'd make sense. Otherwise a library helper would be more appropriate. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC 1/1] net/tls(TLS_SW): Handle -ENOSPC error return from device/AES-NI
On Fri, Oct 09, 2020 at 09:48:30AM -0700, Jakub Kicinski wrote: > > Are you saying that if we set CRYPTO_TFM_REQ_MAY_BACKLOG we should > never see ENOSPC with a correctly functioning driver? Or do we need > to handle both errors, regardless? Correct, you will never see ENOSPC if you request MAY_BACKLOG. However, you must then ensure that when you get EBUSY that you stop issuing new requests until the Crypto API signals through the callback that you can start again. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC 1/1] net/tls(TLS_SW): Handle -ENOSPC error return from device/AES-NI
Jakub Kicinski wrote: > > Why would the driver return EBUSY from an async API? What's the caller > supposed to do with that? The Crypto API offers two modes for callers to deal with congestion. If the request can be safely dropped (e.g., IPsec) then ENOSPC will be returned and should be dealt with accordingly. If the request cannot be dropped then EBUSY is returned to indicate congestion, and the caller must refrain from issuing any more requests until the Crypto API signals that there is space for them. The request flag CRYPTO_TFM_REQ_MAY_BACKLOG is used to indicate which mode you wish to use. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 7/9 net-next] xfrm: use dev_sw_netstats_rx_add()
On Mon, Oct 05, 2020 at 10:36:34PM +0200, Fabian Frederick wrote: > use new helper for netstats settings > > Signed-off-by: Fabian Frederick > --- > net/xfrm/xfrm_interface.c | 9 + > 1 file changed, 1 insertion(+), 8 deletions(-) Acked-by: Herbert Xu -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC PATCH] lsm,selinux: pass the family information along with xfrm flow
On Wed, Sep 30, 2020 at 09:09:20AM +1000, James Morris wrote: > > I'm not keen on adding a parameter which nobody is using. Perhaps a note > in the header instead? Please at least change to the struct flowi to flowi_common if we're not adding a family field. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] xfrm: Use correct address family in xfrm_state_find
Resend with proper subject.
---8<---
The struct flowi must never be interpreted by itself as its size
depends on the address family. Therefore it must always be grouped
with its original family value.
In this particular instance, the original family value is lost in
the function xfrm_state_find. Therefore we get a bogus read when
it's coupled with the wrong family which would occur with inter-
family xfrm states.
This patch fixes it by keeping the original family value.
Note that the same bug could potentially occur in LSM through
the xfrm_state_pol_flow_match hook. I checked the current code
there and it seems to be safe for now as only secid is used which
is part of struct flowi_common. But that API should be changed
so that so that we don't get new bugs in the future. We could
do that by replacing fl with just secid or adding a family field.
Reported-by: syzbot+577fbac3145a6eb2e...@syzkaller.appspotmail.com
Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
Signed-off-by: Herbert Xu
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 69520ad3d83b..9b5f2c2b9770 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1019,7 +1019,8 @@ static void xfrm_state_look_at(struct xfrm_policy *pol,
struct xfrm_state *x,
*/
if (x->km.state == XFRM_STATE_VALID) {
if ((x->sel.family &&
-!xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
+(x->sel.family != family ||
+ !xfrm_selector_match(&x->sel, fl, family))) ||
!security_xfrm_state_pol_flow_match(x, pol, fl))
return;
@@ -1032,7 +1033,9 @@ static void xfrm_state_look_at(struct xfrm_policy *pol,
struct xfrm_state *x,
*acq_in_progress = 1;
} else if (x->km.state == XFRM_STATE_ERROR ||
x->km.state == XFRM_STATE_EXPIRED) {
- if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
+ if ((!x->sel.family ||
+(x->sel.family == family &&
+ xfrm_selector_match(&x->sel, fl, family))) &&
security_xfrm_state_pol_flow_match(x, pol, fl))
*error = -ESRCH;
}
@@ -1072,7 +1075,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const
xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.proto == x->id.proto &&
(tmpl->id.spi == x->id.spi || !tmpl->id.spi))
- xfrm_state_look_at(pol, x, fl, encap_family,
+ xfrm_state_look_at(pol, x, fl, family,
&best, &acquire_in_progress, &error);
}
if (best || acquire_in_progress)
@@ -1089,7 +1092,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const
xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.proto == x->id.proto &&
(tmpl->id.spi == x->id.spi || !tmpl->id.spi))
- xfrm_state_look_at(pol, x, fl, encap_family,
+ xfrm_state_look_at(pol, x, fl, family,
&best, &acquire_in_progress, &error);
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: KASAN: stack-out-of-bounds Read in xfrm_selector_match (2)
On Mon, Sep 21, 2020 at 07:56:20AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:eb5f95f1 Merge tag 's390-5.9-6' of git://git.kernel.org/pu..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13996ad590
> kernel config: https://syzkaller.appspot.com/x/.config?x=ffe85b197a57c180
> dashboard link: https://syzkaller.appspot.com/bug?extid=577fbac3145a6eb2e7a5
> compiler: gcc (GCC) 10.1.0-syz 20200507
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+577fbac3145a6eb2e...@syzkaller.appspotmail.com
>
> ==
> BUG: KASAN: stack-out-of-bounds in xfrm_flowi_dport include/net/xfrm.h:877
> [inline]
> BUG: KASAN: stack-out-of-bounds in __xfrm6_selector_match
> net/xfrm/xfrm_policy.c:216 [inline]
> BUG: KASAN: stack-out-of-bounds in xfrm_selector_match+0xf36/0xf60
> net/xfrm/xfrm_policy.c:229
> Read of size 2 at addr c9001914f55c by task syz-executor.4/15633
>
> CPU: 0 PID: 15633 Comm: syz-executor.4 Not tainted 5.9.0-rc5-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x198/0x1fd lib/dump_stack.c:118
> print_address_description.constprop.0.cold+0x5/0x497 mm/kasan/report.c:383
> __kasan_report mm/kasan/report.c:513 [inline]
> kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
> xfrm_flowi_dport include/net/xfrm.h:877 [inline]
This one goes back more than ten years. This patch should fix
it.
---8<---
The struct flowi must never be interpreted by itself as its size
depends on the address family. Therefore it must always be grouped
with its original family value.
In this particular instance, the original family value is lost in
the function xfrm_state_find. Therefore we get a bogus read when
it's coupled with the wrong family which would occur with inter-
family xfrm states.
This patch fixes it by keeping the original family value.
Note that the same bug could potentially occur in LSM through
the xfrm_state_pol_flow_match hook. I checked the current code
there and it seems to be safe for now as only secid is used which
is part of struct flowi_common. But that API should be changed
so that so that we don't get new bugs in the future. We could
do that by replacing fl with just secid or adding a family field.
Reported-by: syzbot+577fbac3145a6eb2e...@syzkaller.appspotmail.com
Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
Signed-off-by: Herbert Xu
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 69520ad3d83b..9b5f2c2b9770 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1019,7 +1019,8 @@ static void xfrm_state_look_at(struct xfrm_policy *pol,
struct xfrm_state *x,
*/
if (x->km.state == XFRM_STATE_VALID) {
if ((x->sel.family &&
-!xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
+(x->sel.family != family ||
+ !xfrm_selector_match(&x->sel, fl, family))) ||
!security_xfrm_state_pol_flow_match(x, pol, fl))
return;
@@ -1032,7 +1033,9 @@ static void xfrm_state_look_at(struct xfrm_policy *pol,
struct xfrm_state *x,
*acq_in_progress = 1;
} else if (x->km.state == XFRM_STATE_ERROR ||
x->km.state == XFRM_STATE_EXPIRED) {
- if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
+ if ((!x->sel.family ||
+(x->sel.family == family &&
+ xfrm_selector_match(&x->sel, fl, family))) &&
security_xfrm_state_pol_flow_match(x, pol, fl))
*error = -ESRCH;
}
@@ -1072,7 +1075,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const
xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.proto == x->id.proto &&
(tmpl->id.spi == x->id.spi || !tmpl->id.spi))
- xfrm_state_look_at(pol, x, fl, encap_family,
+ xfrm_state_look_at(pol, x, fl, family,
&best, &acquire_in_progress, &error);
}
if (best || acquire_in_progress)
@@ -1089,7 +1092,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const
xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.prot
Re: KASAN: stack-out-of-bounds Read in xfrm_selector_match (2)
On Thu, Sep 24, 2020 at 09:40:26AM +0200, Steffen Klassert wrote: > > This is yet another ipv4 mapped ipv6 address with IPsec socket policy > combination bug, and I'm sure it is not the last one. We could fix this > one by adding another check to match the address family of the policy > and the SA selector, but maybe it is better to think about how this > should work at all. > > We can have only one socket policy for each direction and that > policy accepts either ipv4 or ipv6. We treat this ipv4 mapped ipv6 > address as ipv4 and pass it down the ipv4 stack, so this dual usage > will not work with a socket policy. Maybe we can require IPV6_V6ONLY > for sockets with policy attached. Thoughts? I'm looking at the history of this and it used to work at the start because you'd always interpret the flow object with a family. This appears to have been lost with 8444cf712c5f71845cba9dc30d8f530ff0d5ff83. I'm working on a fix. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: possible deadlock in xfrm_policy_delete
On Thu, Sep 24, 2020 at 09:30:03AM +0200, pet...@infradead.org wrote:
> On Thu, Sep 24, 2020 at 06:44:12AM +0200, Dmitry Vyukov wrote:
> > On Thu, Sep 24, 2020 at 6:36 AM Herbert Xu
> > wrote:
> > > > (k-slock-AF_INET6){+.-.}-{2:2}
>
> That's a seqlock.
Are you sure? Line 2503 in net/ipv4/tcp.c says
bh_lock_sock(sk);
> Did that tree you're testing include 267580db047e ("seqlock: Unbreak
> lockdep") ?
According to
https://syzkaller.appspot.com/bug?extid=c32502fd255cb3a44048
the git tree used was
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include?id=5fa35f247b563a7893f3f68f19d00ace2ccf3dff
so no it doesn't contain that patch.
Hopefully this would address those seqlock backtraces. But what
about this particular one which is caused by the socket lock?
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: possible deadlock in xfrm_policy_delete
On Thu, Sep 24, 2020 at 06:44:12AM +0200, Dmitry Vyukov wrote: > > FWIW one of the dups of this issue was bisected to: > > commit 1909760f5fc3f123e47b4e24e0ccdc0fc8f3f106 > Author: Ahmed S. Darwish > Date: Fri Sep 4 15:32:31 2020 + > > seqlock: PREEMPT_RT: Do not starve seqlock_t writers > > Can it be related? Whether there is a genuine problem or not, the lockdep report that I quoted is simply non-sensical because it's considering two distinct seqlocks to be the same lock. I don't think the lockdep issue has anything to do with the commit question because this commit is specific to seqlocks. There is another syzbot report in this pile that mixed the SCTP socket lock with the TCP socket lock and those are not seqlocks. It's almost as if when a spinlock is freed and reallocated lockdep is not clearing the existing state. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: inconsistent lock state in xfrm_policy_lookup_inexact_addr
On Wed, Sep 16, 2020 at 01:51:14AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:6b02addb Add linux-next specific files for 20200915
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=15888efd90
> kernel config: https://syzkaller.appspot.com/x/.config?x=7086d0e9e44d4a14
> dashboard link: https://syzkaller.appspot.com/bug?extid=f4d0f0f7c860608404c4
> compiler: gcc (GCC) 10.1.0-syz 20200507
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+f4d0f0f7c86060840...@syzkaller.appspotmail.com
>
>
> WARNING: inconsistent lock state
> 5.9.0-rc5-next-20200915-syzkaller #0 Not tainted
>
> inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-R} usage.
> kworker/1:1/23 [HC0[0]:SC1[1]:HE0:SE0] takes:
> 8880a6ff5f28 (&s->seqcount#12){+.+-}-{0:0}, at:
> xfrm_policy_lookup_inexact_addr+0x57/0x200 net/xfrm/xfrm_policy.c:1909
> {SOFTIRQ-ON-W} state was registered at:
> lock_acquire+0x1f2/0xaa0 kernel/locking/lockdep.c:5398
> write_seqcount_t_begin_nested include/linux/seqlock.h:509 [inline]
> write_seqcount_t_begin include/linux/seqlock.h:535 [inline]
> write_seqlock include/linux/seqlock.h:883 [inline]
> xfrm_set_spdinfo+0x302/0x660 net/xfrm/xfrm_user.c:1185
This is &net->xfrm.policy_hthresh.lock.
...
> stack backtrace:
> CPU: 1 PID: 23 Comm: kworker/1:1 Not tainted
> 5.9.0-rc5-next-20200915-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Workqueue: wg-crypt-wg0 wg_packet_tx_worker
> Call Trace:
>
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x198/0x1fb lib/dump_stack.c:118
> print_usage_bug kernel/locking/lockdep.c:3694 [inline]
> valid_state kernel/locking/lockdep.c:3705 [inline]
> mark_lock_irq kernel/locking/lockdep.c:3908 [inline]
> mark_lock.cold+0x13/0x10d kernel/locking/lockdep.c:4375
> mark_usage kernel/locking/lockdep.c:4252 [inline]
> __lock_acquire+0x1402/0x55d0 kernel/locking/lockdep.c:4750
> lock_acquire+0x1f2/0xaa0 kernel/locking/lockdep.c:5398
> seqcount_lockdep_reader_access+0x139/0x1a0 include/linux/seqlock.h:103
> xfrm_policy_lookup_inexact_addr+0x57/0x200 net/xfrm/xfrm_policy.c:1909
And this is a completely different seqlock.
Again lockdep is creating a bogus report by lumping two unrelated
locks (but of the same type, in this case seqlock) together.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: possible deadlock in xfrm_user_rcv_msg
#syz dup: inconsistent lock state in xfrm_policy_lookup_inexact_addr -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: possible deadlock in xfrm_policy_delete
On Sun, Sep 20, 2020 at 01:22:14PM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:5fa35f24 Add linux-next specific files for 20200916
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1122e2d990
> kernel config: https://syzkaller.appspot.com/x/.config?x=6bdb7e39caf48f53
> dashboard link: https://syzkaller.appspot.com/bug?extid=c32502fd255cb3a44048
> compiler: gcc (GCC) 10.1.0-syz 20200507
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+c32502fd255cb3a44...@syzkaller.appspotmail.com
>
> =
> WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
> 5.9.0-rc5-next-20200916-syzkaller #0 Not tainted
> -
> syz-executor.1/13775 [HC0[0]:SC0[4]:HE1:SE0] is trying to acquire:
> 88805ee15a58 (&net->xfrm.xfrm_policy_lock){+...}-{2:2}, at: spin_lock_bh
> include/linux/spinlock.h:359 [inline]
> 88805ee15a58 (&net->xfrm.xfrm_policy_lock){+...}-{2:2}, at:
> xfrm_policy_delete+0x3a/0x90 net/xfrm/xfrm_policy.c:2236
>
> and this task is already holding:
> 8880a811b1e0 (k-slock-AF_INET6){+.-.}-{2:2}, at: spin_lock
> include/linux/spinlock.h:354 [inline]
> 8880a811b1e0 (k-slock-AF_INET6){+.-.}-{2:2}, at: tcp_close+0x6e3/0x1200
> net/ipv4/tcp.c:2503
> which would create a new lock dependency:
> (k-slock-AF_INET6){+.-.}-{2:2} -> (&net->xfrm.xfrm_policy_lock){+...}-{2:2}
>
> but this new dependency connects a SOFTIRQ-irq-safe lock:
> (k-slock-AF_INET6){+.-.}-{2:2}
>
> ... which became SOFTIRQ-irq-safe at:
> lock_acquire+0x1f2/0xaa0 kernel/locking/lockdep.c:5398
> __raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
> _raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:151
> spin_lock include/linux/spinlock.h:354 [inline]
> sctp_rcv+0xd96/0x2d50 net/sctp/input.c:231
What's going on with all these bogus lockdep reports?
These are two completely different locks, one is for TCP and the
other is for SCTP. Why is lockdep suddenly beoming confused about
this?
FWIW this flood of bogus reports started on 16/Sep.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: possible deadlock in xfrm_policy_lookup_bytype
#syz dup: inconsistent lock state in xfrm_policy_lookup_inexact_addr -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: WARNING: SOFTIRQ-READ-safe -> SOFTIRQ-READ-unsafe lock order detected
#syz dup: inconsistent lock state in xfrm_policy_lookup_inexact_addr -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: inconsistent lock state in xfrm_user_rcv_msg
#syz dup: inconsistent lock state in xfrm_policy_lookup_inexact_addr -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: possible deadlock in xfrm_policy_lookup_inexact_addr
#syz dup: inconsistent lock state in xfrm_policy_lookup_inexact_addr -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] rhashtable: fix indentation of a continue statement
On Fri, Sep 18, 2020 at 10:51:26PM +0100, Colin King wrote: > From: Colin Ian King > > A continue statement is indented incorrectly, add in the missing > tab. > > Signed-off-by: Colin Ian King > --- > lib/test_rhashtable.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Herbert Xu -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v3 0/7] crypto: mark ecb(arc4) skcipher as obsolete
On Mon, Aug 31, 2020 at 06:16:42PM +0300, Ard Biesheuvel wrote: > RC4 hasn't aged very well, and is a poor fit for the skcipher API so it > would be good if we could get rid of the ecb(arc4) drivers in the kernel > at some point in the future. This prevents new users from creeping in, and > allows us to improve the skcipher API without having to care too much about > obsolete algorithms that may be difficult to support going forward. > > So let's get rid of any remaining in-kernel users, either by switching them > to the arc4 library API (for cases which simply cannot change algorithms, > e.g., WEP), or dropping the code entirely. Also remove the remaining h/w > accelerated implementations, and mark the generic s/w implementation as > obsolete in Kconfig. > > Changes since v2: > - depend on CRYPTO_USER_API not CRYPTO_USER > - rename CRYPTO_USER_ENABLE_OBSOLETE to CRYPTO_USER_API_ENABLE_OBSOLETE for > clarity > > Changes since RFC [0]: > - keep ecb(arc4) generic C implementation, and the associated test vectors, > but print a warning about ecb(arc4) being obsolete so we can identify > remaining users > - add a Kconfig option to en/disable obsolete algorithms that are only kept > around to prevent breaking users that rely on it via the socket interface > - add a patch to clean up some bogus Kconfig dependencies > - add acks to patches #1, #2 and #3 > > [0] > https://lore.kernel.org/driverdev-devel/20200702101947.682-1-a...@kernel.org/ > > Cc: Herbert Xu > Cc: "David S. Miller" > Cc: Greg Kroah-Hartman > Cc: Trond Myklebust > Cc: Anna Schumaker > Cc: "J. Bruce Fields" > Cc: Chuck Lever > Cc: Eric Biggers > Cc: Arnd Bergmann > Cc: linux-cry...@vger.kernel.org > Cc: netdev@vger.kernel.org > Cc: de...@driverdev.osuosl.org > Cc: linux-...@vger.kernel.org > > Ard Biesheuvel (7): > staging/rtl8192e: switch to RC4 library interface > staging/rtl8192u: switch to RC4 library interface > SUNRPC: remove RC4-HMAC-MD5 support from KerberosV > crypto: n2 - remove ecb(arc4) support > crypto: bcm-iproc - remove ecb(arc4) support > net: wireless: drop bogus CRYPTO_xxx Kconfig selects > crypto: arc4 - mark ecb(arc4) skcipher as obsolete > > crypto/Kconfig| 10 + > crypto/arc4.c | 10 + > drivers/crypto/bcm/cipher.c | 96 +- > drivers/crypto/bcm/cipher.h | 1 - > drivers/crypto/bcm/spu.c | 23 +- > drivers/crypto/bcm/spu.h | 1 - > drivers/crypto/bcm/spu2.c | 12 +- > drivers/crypto/bcm/spu2.h | 1 - > drivers/crypto/n2_core.c | 46 --- > drivers/net/wireless/intel/ipw2x00/Kconfig| 4 - > drivers/net/wireless/intersil/hostap/Kconfig | 4 - > drivers/staging/rtl8192e/Kconfig | 4 +- > drivers/staging/rtl8192e/rtllib_crypt_tkip.c | 70 + > drivers/staging/rtl8192e/rtllib_crypt_wep.c | 72 + > drivers/staging/rtl8192u/Kconfig | 1 + > .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 81 + > .../rtl8192u/ieee80211/ieee80211_crypt_wep.c | 64 +--- > include/linux/sunrpc/gss_krb5.h | 11 - > include/linux/sunrpc/gss_krb5_enctypes.h | 9 +- > net/sunrpc/Kconfig| 1 - > net/sunrpc/auth_gss/gss_krb5_crypto.c | 276 -- > net/sunrpc/auth_gss/gss_krb5_mech.c | 95 -- > net/sunrpc/auth_gss/gss_krb5_seal.c | 1 - > net/sunrpc/auth_gss/gss_krb5_seqnum.c | 87 -- > net/sunrpc/auth_gss/gss_krb5_unseal.c | 1 - > net/sunrpc/auth_gss/gss_krb5_wrap.c | 65 + > 26 files changed, 97 insertions(+), 949 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v3 7/7] crypto: arc4 - mark ecb(arc4) skcipher as obsolete
On Mon, Aug 31, 2020 at 06:16:49PM +0300, Ard Biesheuvel wrote: > > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include This needs kernel.h too for the pr_warn_ratelimited. I'll add it when I apply the series. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [EXT] Re: [PATCH v2 2/3] drivers: crypto: add support for OCTEONTX2 CPT engine
On Fri, Sep 04, 2020 at 02:14:34PM +, Srujana Challa wrote: > > Since LMT store is our platform specific, it cannot be generalized to all > ARM64. I'm not asking you to generalise it to all of ARM64. I'm asking you to move this into a header file under arch/arm64 that can then be shared by both your crypto driver and your network driver so you don't duplicate this everywhere. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 2/3] drivers: crypto: add support for OCTEONTX2 CPT engine
On Fri, Sep 04, 2020 at 01:45:38PM +, Srujana Challa wrote: > > This block of code is used for LMT store operations. The LMT store operation > is specific to our platform, and this uses the "ldeor" instruction(which is > actually an LSE atomic instruction available on v8.1 CPUs) targeting the > IO address. > We add it in the driver since we want LMT store to work even if LSE_ATOMICS > is disabled. You have exactly the same macro in your net driver. Move it into a header file in arch/arm64/include/asm and also add one under include/asm-generic so we can compile-test. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 7/7] crypto: arc4 - mark ecb(arc4) skcipher as obsolete
On Mon, Aug 24, 2020 at 03:30:01PM +0200, Ard Biesheuvel wrote: > > +config CRYPTO_USER_ENABLE_OBSOLETE > + bool "Enable obsolete cryptographic algorithms for userspace" > + depends on CRYPTO_USER That should be CRYPTO_USER_API which is the option for af_alg. CRYPTO_USER is the configuration interface which has nothing to do with af_alg. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] net: Get rid of consume_skb when tracing is off
On Sat, Aug 22, 2020 at 01:54:19PM -0400, Neil Horman wrote: > > Wouldn't it be better to make this: > #define consume_skb(x) kfree_skb(x) Either way is fine but I prefer inline functions over macros. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] net: Get rid of consume_skb when tracing is off
On Fri, Aug 21, 2020 at 04:40:49PM -0700, Eric Dumazet wrote: > > I am not completely familiar with CONFIG_TRACEPOINTS > > Is "perf probe" support requiring it ? Yes. perf probe requires CONFIG_KPROBE_EVENTS which selects CONFIG_TRACING which selects CONFIG_TRACEPOINTS. > We want the following to be supported. > > perf probe consume_skb That should continue to work as this patch does not change anything when CONFIG_TRACEPOINTS is enabled. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] net: Get rid of consume_skb when tracing is off
The function consume_skb is only meaningful when tracing is enabled.
This patch makes it conditional on CONFIG_TRACEPOINTS.
Signed-off-by: Herbert Xu
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 46881d902124..e8bca74857a3 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1056,7 +1056,16 @@ void kfree_skb(struct sk_buff *skb);
void kfree_skb_list(struct sk_buff *segs);
void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt);
void skb_tx_error(struct sk_buff *skb);
+
+#ifdef CONFIG_TRACEPOINTS
void consume_skb(struct sk_buff *skb);
+#else
+static inline void consume_skb(struct sk_buff *skb)
+{
+ return kfree_skb(skb);
+}
+#endif
+
void __consume_stateless_skb(struct sk_buff *skb);
void __kfree_skb(struct sk_buff *skb);
extern struct kmem_cache *skbuff_head_cache;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7e2e502ef519..593fe73d4993 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -820,6 +820,7 @@ void skb_tx_error(struct sk_buff *skb)
}
EXPORT_SYMBOL(skb_tx_error);
+#ifdef CONFIG_TRACEPOINTS
/**
* consume_skb - free an skbuff
* @skb: buffer to free
@@ -837,6 +838,7 @@ void consume_skb(struct sk_buff *skb)
__kfree_skb(skb);
}
EXPORT_SYMBOL(consume_skb);
+#endif
/**
* consume_stateless_skb - free an skbuff, assuming it is stateless
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: hifn_795x - switch from 'pci_' to 'dma_' API
On Mon, Jul 27, 2020 at 11:30:27AM +0200, Christophe JAILLET wrote: > The wrappers in include/linux/pci-dma-compat.h should go away. > > The patch has been generated with the coccinelle script below and has been > hand modified to replace GFP_ with a correct flag. > It has been compile tested. > > When memory is allocated in 'hifn_probe()' GFP_KERNEL can be used > because it is a probe function and no spin_lock is taken. > > @@ > @@ > -PCI_DMA_BIDIRECTIONAL > +DMA_BIDIRECTIONAL > > @@ > @@ > -PCI_DMA_TODEVICE > +DMA_TO_DEVICE > > @@ > @@ > -PCI_DMA_FROMDEVICE > +DMA_FROM_DEVICE > > @@ > @@ > -PCI_DMA_NONE > +DMA_NONE > > @@ > expression e1, e2, e3; > @@ > -pci_alloc_consistent(e1, e2, e3) > +dma_alloc_coherent(&e1->dev, e2, e3, GFP_) > > @@ > expression e1, e2, e3; > @@ > -pci_zalloc_consistent(e1, e2, e3) > +dma_alloc_coherent(&e1->dev, e2, e3, GFP_) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_free_consistent(e1, e2, e3, e4) > +dma_free_coherent(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_map_single(e1, e2, e3, e4) > +dma_map_single(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_unmap_single(e1, e2, e3, e4) > +dma_unmap_single(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4, e5; > @@ > -pci_map_page(e1, e2, e3, e4, e5) > +dma_map_page(&e1->dev, e2, e3, e4, e5) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_unmap_page(e1, e2, e3, e4) > +dma_unmap_page(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_map_sg(e1, e2, e3, e4) > +dma_map_sg(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_unmap_sg(e1, e2, e3, e4) > +dma_unmap_sg(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_dma_sync_single_for_cpu(e1, e2, e3, e4) > +dma_sync_single_for_cpu(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_dma_sync_single_for_device(e1, e2, e3, e4) > +dma_sync_single_for_device(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_dma_sync_sg_for_cpu(e1, e2, e3, e4) > +dma_sync_sg_for_cpu(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2, e3, e4; > @@ > -pci_dma_sync_sg_for_device(e1, e2, e3, e4) > +dma_sync_sg_for_device(&e1->dev, e2, e3, e4) > > @@ > expression e1, e2; > @@ > -pci_dma_mapping_error(e1, e2) > +dma_mapping_error(&e1->dev, e2) > > @@ > expression e1, e2; > @@ > -pci_set_dma_mask(e1, e2) > +dma_set_mask(&e1->dev, e2) > > @@ > expression e1, e2; > @@ > -pci_set_consistent_dma_mask(e1, e2) > +dma_set_coherent_mask(&e1->dev, e2) > > Signed-off-by: Christophe JAILLET > --- > If needed, see post from Christoph Hellwig on the kernel-janitors ML: >https://marc.info/?l=kernel-janitors&m=158745678307186&w=4 > --- > drivers/crypto/hifn_795x.c | 21 - > 1 file changed, 12 insertions(+), 9 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 3/3] drivers: crypto: add the Virtual Function driver for OcteonTX2 CPT
On Fri, Aug 07, 2020 at 07:39:20PM +0530, Srujana Challa wrote:
>
> +static inline int is_any_alg_used(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(otx2_cpt_skciphers); i++)
> + if (refcount_read(&otx2_cpt_skciphers[i].base.cra_refcnt) != 1)
> + return true;
> + for (i = 0; i < ARRAY_SIZE(otx2_cpt_aeads); i++)
> + if (refcount_read(&otx2_cpt_aeads[i].base.cra_refcnt) != 1)
> + return true;
> + return false;
> +}
This is racy as there is nothing stopping new users from coming in
after you've finished the test.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 2/3] drivers: crypto: add support for OCTEONTX2 CPT engine
On Fri, Aug 07, 2020 at 07:39:19PM +0530, Srujana Challa wrote:
>
> +#if defined(CONFIG_ARM64)
> +static inline long otx2_lmt_flush(void *ioreg)
> +{
> + long result = 0;
> +
> + __asm__ volatile(".cpu generic+lse\n"
> + "ldeor xzr, %0, [%1]\n"
> + : "=r" (result)
> + : "r" (ioreg) : "memory");
> +
> + return result;
> +}
> +
> +#else
> +#define otx2_lmt_flush(addr) ({ 0; })
> +#endif
This is not acceptable. Please work out a way with the ARM folks
to fix this without adding assembly code in a driver.
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 2/3] drivers: crypto: add support for OCTEONTX2 CPT engine
On Fri, Aug 07, 2020 at 07:39:19PM +0530, Srujana Challa wrote:
>
> +/*
> + * On OcteonTX2 platform the parameter insts_num is used as a count of
> + * instructions to be enqueued. The valid values for insts_num are:
> + * 1 - 1 CPT instruction will be enqueued during LMTST operation
> + * 2 - 2 CPT instructions will be enqueued during LMTST operation
> + */
> +static inline void otx2_cpt_send_cmd(union otx2_cpt_inst_s *cptinst,
> + u32 insts_num, void *obj)
> +{
> + struct otx2_cptlf_info *lf = obj;
> + void *lmtline = lf->lmtline;
> + long ret;
> +
> + /*
> + * Make sure memory areas pointed in CPT_INST_S
> + * are flushed before the instruction is sent to CPT
> + */
> + smp_wmb();
Why should this be a NOOP on UP?
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH ipsec-next] xfrm: add /proc/sys/core/net/xfrm_redact_secret
On Tue, Jul 28, 2020 at 05:47:30PM +0200, Antony Antony wrote: > when enabled, 1, redact XFRM SA secret in the netlink response to > xfrm_get_sa() or dump all sa. > > e.g > echo 1 > /proc/sys/net/core/xfrm_redact_secret > ip xfrm state > src 172.16.1.200 dst 172.16.1.100 > proto esp spi 0x0002 reqid 2 mode tunnel > replay-window 0 > aead rfc4106(gcm(aes)) 0x 96 > > the aead secret is redacted. > > /proc/sys/core/net/xfrm_redact_secret is a toggle. > Once enabled, either at compile or via proc, it can not be disabled. > Redacting secret is a FIPS 140-2 requirement. Couldn't you use the existing fips_enabled sysctl? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net V2] Crypto/chcr: Registering cxgb4 to xfrmdev_ops
On Fri, Jul 24, 2020 at 05:01:08PM -0700, David Miller wrote: > > Please start submitting chcr patches to the crypto subsystem, where it > belongs, instead of the networking GIT trees. Hi Dave: I think this patch belongs to the networking tree. The reason is that it's related to xfrm offload which has nothing to do with the Crypto API. Do xfrm offload drivers usually go through the networking tree or would it be better directed through the xfrm tree? There's really nobody on the crypto mailing list who could give this the proper review that it deserves. Of course I'm happy to continue taking anything that touches chcr_algo.c as that resides wholly within the Crypto API. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v2 PATCH 2/2] rhashtable: Restore RCU marking on rhash_lock_head
This patch restores the RCU marking on bucket_table->buckets as
it really does need RCU protection. Its removal had led to a fatal
bug.
Signed-off-by: Herbert Xu
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index e3def7bbe932..83ad875a7ea2 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -84,7 +84,7 @@ struct bucket_table {
struct lockdep_map dep_map;
- struct rhash_lock_head *buckets[] cacheline_aligned_in_smp;
+ struct rhash_lock_head __rcu *buckets[] cacheline_aligned_in_smp;
};
/*
@@ -261,13 +261,12 @@ void rhashtable_free_and_destroy(struct rhashtable *ht,
void *arg);
void rhashtable_destroy(struct rhashtable *ht);
-struct rhash_lock_head **rht_bucket_nested(const struct bucket_table *tbl,
- unsigned int hash);
-struct rhash_lock_head **__rht_bucket_nested(const struct bucket_table *tbl,
-unsigned int hash);
-struct rhash_lock_head **rht_bucket_nested_insert(struct rhashtable *ht,
- struct bucket_table *tbl,
- unsigned int hash);
+struct rhash_lock_head __rcu **rht_bucket_nested(
+ const struct bucket_table *tbl, unsigned int hash);
+struct rhash_lock_head __rcu **__rht_bucket_nested(
+ const struct bucket_table *tbl, unsigned int hash);
+struct rhash_lock_head __rcu **rht_bucket_nested_insert(
+ struct rhashtable *ht, struct bucket_table *tbl, unsigned int hash);
#define rht_dereference(p, ht) \
rcu_dereference_protected(p, lockdep_rht_mutex_is_held(ht))
@@ -284,21 +283,21 @@ struct rhash_lock_head **rht_bucket_nested_insert(struct
rhashtable *ht,
#define rht_entry(tpos, pos, member) \
({ tpos = container_of(pos, typeof(*tpos), member); 1; })
-static inline struct rhash_lock_head *const *rht_bucket(
+static inline struct rhash_lock_head __rcu *const *rht_bucket(
const struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? rht_bucket_nested(tbl, hash) :
&tbl->buckets[hash];
}
-static inline struct rhash_lock_head **rht_bucket_var(
+static inline struct rhash_lock_head __rcu **rht_bucket_var(
struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? __rht_bucket_nested(tbl, hash) :
&tbl->buckets[hash];
}
-static inline struct rhash_lock_head **rht_bucket_insert(
+static inline struct rhash_lock_head __rcu **rht_bucket_insert(
struct rhashtable *ht, struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? rht_bucket_nested_insert(ht, tbl, hash) :
@@ -325,7 +324,7 @@ static inline struct rhash_lock_head **rht_bucket_insert(
*/
static inline void rht_lock(struct bucket_table *tbl,
- struct rhash_lock_head **bkt)
+ struct rhash_lock_head __rcu **bkt)
{
local_bh_disable();
bit_spin_lock(0, (unsigned long *)bkt);
@@ -333,7 +332,7 @@ static inline void rht_lock(struct bucket_table *tbl,
}
static inline void rht_lock_nested(struct bucket_table *tbl,
- struct rhash_lock_head **bucket,
+ struct rhash_lock_head __rcu **bucket,
unsigned int subclass)
{
local_bh_disable();
@@ -342,7 +341,7 @@ static inline void rht_lock_nested(struct bucket_table *tbl,
}
static inline void rht_unlock(struct bucket_table *tbl,
- struct rhash_lock_head **bkt)
+ struct rhash_lock_head __rcu **bkt)
{
lock_map_release(&tbl->dep_map);
bit_spin_unlock(0, (unsigned long *)bkt);
@@ -365,48 +364,41 @@ static inline struct rhash_head *__rht_ptr(
*access is guaranteed, such as when destroying the table.
*/
static inline struct rhash_head *rht_ptr_rcu(
- struct rhash_lock_head *const *p)
+ struct rhash_lock_head __rcu *const *bkt)
{
- struct rhash_lock_head __rcu *const *bkt = (void *)p;
return __rht_ptr(rcu_dereference(*bkt), bkt);
}
static inline struct rhash_head *rht_ptr(
- struct rhash_lock_head *const *p,
+ struct rhash_lock_head __rcu *const *bkt,
struct bucket_table *tbl,
unsigned int hash)
{
- struct rhash_lock_head __rcu *const *bkt = (void *)p;
return __rht_ptr(rht_dereference_bucket(*bkt, tbl, hash), bkt);
}
static inline struct rhash_head *rht_ptr_exclusive(
- struct rhash_lock_head *const *p)
+ struct rhash_lock_head __rcu *const *bkt)
{
- struct rhash_lock_head __rcu *const *bkt = (void *)p;
return __rht_ptr(rcu_dereference_protected(*bkt, 1), bkt);
}
-static inline void rht_ass
[v2 PATCH 1/2] rhashtable: Fix unprotected RCU dereference in __rht_ptr
The rcu_dereference call in rht_ptr_rcu is completely bogus because
we've already dereferenced the value in __rht_ptr and operated on it.
This causes potential double readings which could be fatal. The RCU
dereference must occur prior to the comparison in __rht_ptr.
This patch changes the order of RCU dereference so that it is done
first and the result is then fed to __rht_ptr. The RCU marking
changes have been minimised using casts which will be removed in
a follow-up patch.
Fixes: ba6306e3f648 ("rhashtable: Remove RCU marking from...")
Reported-by: "Gong, Sishuai"
Signed-off-by: Herbert Xu
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 70ebef866cc8..e3def7bbe932 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -349,11 +349,11 @@ static inline void rht_unlock(struct bucket_table *tbl,
local_bh_enable();
}
-static inline struct rhash_head __rcu *__rht_ptr(
- struct rhash_lock_head *const *bkt)
+static inline struct rhash_head *__rht_ptr(
+ struct rhash_lock_head *p, struct rhash_lock_head __rcu *const *bkt)
{
- return (struct rhash_head __rcu *)
- ((unsigned long)*bkt & ~BIT(0) ?:
+ return (struct rhash_head *)
+ ((unsigned long)p & ~BIT(0) ?:
(unsigned long)RHT_NULLS_MARKER(bkt));
}
@@ -365,25 +365,26 @@ static inline struct rhash_head __rcu *__rht_ptr(
*access is guaranteed, such as when destroying the table.
*/
static inline struct rhash_head *rht_ptr_rcu(
- struct rhash_lock_head *const *bkt)
+ struct rhash_lock_head *const *p)
{
- struct rhash_head __rcu *p = __rht_ptr(bkt);
-
- return rcu_dereference(p);
+ struct rhash_lock_head __rcu *const *bkt = (void *)p;
+ return __rht_ptr(rcu_dereference(*bkt), bkt);
}
static inline struct rhash_head *rht_ptr(
- struct rhash_lock_head *const *bkt,
+ struct rhash_lock_head *const *p,
struct bucket_table *tbl,
unsigned int hash)
{
- return rht_dereference_bucket(__rht_ptr(bkt), tbl, hash);
+ struct rhash_lock_head __rcu *const *bkt = (void *)p;
+ return __rht_ptr(rht_dereference_bucket(*bkt, tbl, hash), bkt);
}
static inline struct rhash_head *rht_ptr_exclusive(
- struct rhash_lock_head *const *bkt)
+ struct rhash_lock_head *const *p)
{
- return rcu_dereference_protected(__rht_ptr(bkt), 1);
+ struct rhash_lock_head __rcu *const *bkt = (void *)p;
+ return __rht_ptr(rcu_dereference_protected(*bkt, 1), bkt);
}
static inline void rht_assign_locked(struct rhash_lock_head **bkt,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v2 PATCH 0/2] rhashtable: Fix unprotected RCU dereference in __rht_ptr
v2 Added another missing __rcu marker causing warnings. -- This patch series fixes an unprotected dereference in __rht_ptr. The first patch is a minimal fix that does not use the correct RCU markings but is suitable for backport, and the second patch cleans up the RCU markings. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH 2/2] rhashtable: Restore RCU marking on rhash_lock_head
This patch restores the RCU marking on bucket_table->buckets as
it really does need RCU protection. Its removal had led to a fatal
bug.
Signed-off-by: Herbert Xu
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index e3def7bbe932..9a8d4b9dde50 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -84,7 +84,7 @@ struct bucket_table {
struct lockdep_map dep_map;
- struct rhash_lock_head *buckets[] cacheline_aligned_in_smp;
+ struct rhash_lock_head __rcu *buckets[] cacheline_aligned_in_smp;
};
/*
@@ -261,13 +261,12 @@ void rhashtable_free_and_destroy(struct rhashtable *ht,
void *arg);
void rhashtable_destroy(struct rhashtable *ht);
-struct rhash_lock_head **rht_bucket_nested(const struct bucket_table *tbl,
- unsigned int hash);
-struct rhash_lock_head **__rht_bucket_nested(const struct bucket_table *tbl,
-unsigned int hash);
-struct rhash_lock_head **rht_bucket_nested_insert(struct rhashtable *ht,
- struct bucket_table *tbl,
- unsigned int hash);
+struct rhash_lock_head __rcu **rht_bucket_nested(
+ const struct bucket_table *tbl, unsigned int hash);
+struct rhash_lock_head __rcu **__rht_bucket_nested(
+ const struct bucket_table *tbl, unsigned int hash);
+struct rhash_lock_head __rcu **rht_bucket_nested_insert(
+ struct rhashtable *ht, struct bucket_table *tbl, unsigned int hash);
#define rht_dereference(p, ht) \
rcu_dereference_protected(p, lockdep_rht_mutex_is_held(ht))
@@ -284,21 +283,21 @@ struct rhash_lock_head **rht_bucket_nested_insert(struct
rhashtable *ht,
#define rht_entry(tpos, pos, member) \
({ tpos = container_of(pos, typeof(*tpos), member); 1; })
-static inline struct rhash_lock_head *const *rht_bucket(
+static inline struct rhash_lock_head __rcu *const *rht_bucket(
const struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? rht_bucket_nested(tbl, hash) :
&tbl->buckets[hash];
}
-static inline struct rhash_lock_head **rht_bucket_var(
+static inline struct rhash_lock_head __rcu **rht_bucket_var(
struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? __rht_bucket_nested(tbl, hash) :
&tbl->buckets[hash];
}
-static inline struct rhash_lock_head **rht_bucket_insert(
+static inline struct rhash_lock_head __rcu **rht_bucket_insert(
struct rhashtable *ht, struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? rht_bucket_nested_insert(ht, tbl, hash) :
@@ -325,7 +324,7 @@ static inline struct rhash_lock_head **rht_bucket_insert(
*/
static inline void rht_lock(struct bucket_table *tbl,
- struct rhash_lock_head **bkt)
+ struct rhash_lock_head __rcu **bkt)
{
local_bh_disable();
bit_spin_lock(0, (unsigned long *)bkt);
@@ -333,7 +332,7 @@ static inline void rht_lock(struct bucket_table *tbl,
}
static inline void rht_lock_nested(struct bucket_table *tbl,
- struct rhash_lock_head **bucket,
+ struct rhash_lock_head __rcu **bucket,
unsigned int subclass)
{
local_bh_disable();
@@ -342,7 +341,7 @@ static inline void rht_lock_nested(struct bucket_table *tbl,
}
static inline void rht_unlock(struct bucket_table *tbl,
- struct rhash_lock_head **bkt)
+ struct rhash_lock_head __rcu **bkt)
{
lock_map_release(&tbl->dep_map);
bit_spin_unlock(0, (unsigned long *)bkt);
@@ -365,48 +364,41 @@ static inline struct rhash_head *__rht_ptr(
*access is guaranteed, such as when destroying the table.
*/
static inline struct rhash_head *rht_ptr_rcu(
- struct rhash_lock_head *const *p)
+ struct rhash_lock_head __rcu *const *bkt)
{
- struct rhash_lock_head __rcu *const *bkt = (void *)p;
return __rht_ptr(rcu_dereference(*bkt), bkt);
}
static inline struct rhash_head *rht_ptr(
- struct rhash_lock_head *const *p,
+ struct rhash_lock_head __rcu *const *bkt,
struct bucket_table *tbl,
unsigned int hash)
{
- struct rhash_lock_head __rcu *const *bkt = (void *)p;
return __rht_ptr(rht_dereference_bucket(*bkt, tbl, hash), bkt);
}
static inline struct rhash_head *rht_ptr_exclusive(
- struct rhash_lock_head *const *p)
+ struct rhash_lock_head __rcu *const *bkt)
{
- struct rhash_lock_head __rcu *const *bkt = (void *)p;
return __rht_ptr(rcu_dereference_protected(*bkt, 1), bkt);
}
-static inline void rht_ass
[PATCH 1/2] rhashtable: Fix unprotected RCU dereference in __rht_ptr
The rcu_dereference call in rht_ptr_rcu is completely bogus because
we've already dereferenced the value in __rht_ptr and operated on it.
This causes potential double readings which could be fatal. The RCU
dereference must occur prior to the comparison in __rht_ptr.
This patch changes the order of RCU dereference so that it is done
first and the result is then fed to __rht_ptr. The RCU marking
changes have been minimised using casts which will be removed in
a follow-up patch.
Fixes: ba6306e3f648 ("rhashtable: Remove RCU marking from...")
Reported-by: "Gong, Sishuai"
Signed-off-by: Herbert Xu
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 70ebef866cc8..e3def7bbe932 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -349,11 +349,11 @@ static inline void rht_unlock(struct bucket_table *tbl,
local_bh_enable();
}
-static inline struct rhash_head __rcu *__rht_ptr(
- struct rhash_lock_head *const *bkt)
+static inline struct rhash_head *__rht_ptr(
+ struct rhash_lock_head *p, struct rhash_lock_head __rcu *const *bkt)
{
- return (struct rhash_head __rcu *)
- ((unsigned long)*bkt & ~BIT(0) ?:
+ return (struct rhash_head *)
+ ((unsigned long)p & ~BIT(0) ?:
(unsigned long)RHT_NULLS_MARKER(bkt));
}
@@ -365,25 +365,26 @@ static inline struct rhash_head __rcu *__rht_ptr(
*access is guaranteed, such as when destroying the table.
*/
static inline struct rhash_head *rht_ptr_rcu(
- struct rhash_lock_head *const *bkt)
+ struct rhash_lock_head *const *p)
{
- struct rhash_head __rcu *p = __rht_ptr(bkt);
-
- return rcu_dereference(p);
+ struct rhash_lock_head __rcu *const *bkt = (void *)p;
+ return __rht_ptr(rcu_dereference(*bkt), bkt);
}
static inline struct rhash_head *rht_ptr(
- struct rhash_lock_head *const *bkt,
+ struct rhash_lock_head *const *p,
struct bucket_table *tbl,
unsigned int hash)
{
- return rht_dereference_bucket(__rht_ptr(bkt), tbl, hash);
+ struct rhash_lock_head __rcu *const *bkt = (void *)p;
+ return __rht_ptr(rht_dereference_bucket(*bkt, tbl, hash), bkt);
}
static inline struct rhash_head *rht_ptr_exclusive(
- struct rhash_lock_head *const *bkt)
+ struct rhash_lock_head *const *p)
{
- return rcu_dereference_protected(__rht_ptr(bkt), 1);
+ struct rhash_lock_head __rcu *const *bkt = (void *)p;
+ return __rht_ptr(rcu_dereference_protected(*bkt, 1), bkt);
}
static inline void rht_assign_locked(struct rhash_lock_head **bkt,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH 0/2] rhashtable: Fix unprotected RCU dereference in __rht_ptr
This patch series fixes an unprotected dereference in __rht_ptr. The first patch is a minimal fix that does not use the correct RCU markings but is suitable for backport, and the second patch cleans up the RCU markings. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: PROBLEM: potential concurrency bug in rhashtable.h
On Thu, Jul 23, 2020 at 05:34:15PM -0700, Eric Dumazet wrote: > > Sure, but __rht_ptr() is used with different RCU checks, > I guess a that adding these lockdep conditions will make > a patch more invasive. Yes it is large but the only substantial change is to __rht_ptr and its callers. Everything else is just juggling RCU markings. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] flow_offload: Move rhashtable inclusion to the source file
I noticed that touching linux/rhashtable.h causes lib/vsprintf.c to
be rebuilt. This dependency came through a bogus inclusion in the
file net/flow_offload.h. This patch moves it to the right place.
This patch also removes a lingering rhashtable inclusion in cls_api
created by the same commit.
Fixes: 4e481908c51b ("flow_offload: move tc indirect block to...")
Signed-off-by: Herbert Xu
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index f2c8311a0433..1075369d21d3 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -5,7 +5,6 @@
#include
#include
#include
-#include
struct flow_match {
struct flow_dissector *dissector;
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 0cfc35e6be28..e88320c17665 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -4,6 +4,7 @@
#include
#include
#include
+#include
struct flow_rule *flow_rule_alloc(unsigned int num_actions)
{
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a00a203b2ef5..caa254ece49f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -20,7 +20,6 @@
#include
#include
#include
-#include
#include
#include
#include
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: PROBLEM: potential concurrency bug in rhashtable.h
On Thu, Jul 23, 2020 at 03:32:05PM -0700, Eric Dumazet wrote: > > Thanks for the report/analysis. Thanks indeed. > READ_ONCE() should help here, can you test/submit an official patch ? This is basically a hand-rolled RCU access. So we should instead use proper RCU operators if possible. Let me see what I can do. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers
On Wed, Jul 01, 2020 at 11:43:04AM -0700, Eric Dumazet wrote:
> My prior fix went a bit too far, according to Herbert and Mathieu.
>
> Since we accept that concurrent TCP MD5 lookups might see inconsistent
> keys, we can use READ_ONCE()/WRITE_ONCE() instead of smp_rmb()/smp_wmb()
>
> Clearing all key->key[] is needed to avoid possible KMSAN reports,
> if key->keylen is increased. Since tcp_md5_do_add() is not fast path,
> using __GFP_ZERO to clear all struct tcp_md5sig_key is simpler.
>
> data_race() was added in linux-5.8 and will prevent KCSAN reports,
> this can safely be removed in stable backports, if data_race() is
> not yet backported.
>
> v2: use data_race() both in tcp_md5_hash_key() and tcp_md5_do_add()
>
> Fixes: 6a2febec338d ("tcp: md5: add missing memory barriers in
> tcp_md5_do_add()/tcp_md5_hash_key()")
> Signed-off-by: Eric Dumazet
> Cc: Mathieu Desnoyers
> Cc: Herbert Xu
> Cc: Marco Elver
> ---
> net/ipv4/tcp.c | 8
> net/ipv4/tcp_ipv4.c | 19 ++++++-
> 2 files changed, 18 insertions(+), 9 deletions(-)
Acked-by: Herbert Xu
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [regression] TCP_MD5SIG on established sockets
On Tue, Jun 30, 2020 at 08:36:51PM -0700, Eric Dumazet wrote: > > If I knew so many people were excited about TCP / MD5, I would have > posted all my patches on lkml ;) > > Without the smp_wmb() we would still need something to prevent KMSAN > from detecting that we read uninitialized bytes, > if key->keylen is increased. (initial content of key->key[] is garbage) > > Something like this : LGTM. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [regression] TCP_MD5SIG on established sockets
On Tue, Jun 30, 2020 at 07:30:43PM -0700, Eric Dumazet wrote: > > I made this clear in the changelog, do we want comments all over the places ? > Do not get me wrong, we had this bug for years and suddenly this is a > big deal... I thought you were adding a new pair of smp_rmb/smp_wmb. If they already exist in the code then I agree it's not a big deal. But adding a new pair of bogus smp_Xmb's is bad for maintenance. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [regression] TCP_MD5SIG on established sockets
On Tue, Jun 30, 2020 at 07:17:46PM -0700, Eric Dumazet wrote: > > The main issue of the prior code was the double read of key->keylen in > tcp_md5_hash_key(), not that few bytes could change under us. > > I used smp_rmb() to ease backports, since old kernels had no > READ_ONCE()/WRITE_ONCE(), but ACCESS_ONCE() instead. If it's the double-read that you're protecting against, you should just use barrier() and the comment should say so too. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [regression] TCP_MD5SIG on established sockets
Eric Dumazet wrote:
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index
> 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..ecc0e3fabce8b03bef823cbfc5c1b0a9e24df124
> 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4034,9 +4034,12 @@ EXPORT_SYMBOL(tcp_md5_hash_skb_data);
> int tcp_md5_hash_key(struct tcp_md5sig_pool *hp, const struct
> tcp_md5sig_key *key)
> {
>struct scatterlist sg;
> + u8 keylen = key->keylen;
>
> - sg_init_one(&sg, key->key, key->keylen);
> - ahash_request_set_crypt(hp->md5_req, &sg, NULL, key->keylen);
> + smp_rmb(); /* paired with smp_wmb() in tcp_md5_do_add() */
> +
> + sg_init_one(&sg, key->key, keylen);
> + ahash_request_set_crypt(hp->md5_req, &sg, NULL, keylen);
>return crypto_ahash_update(hp->md5_req);
> }
> EXPORT_SYMBOL(tcp_md5_hash_key);
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index
> ad6435ba6d72ffd8caf783bb25cad7ec151d6909..99916fcc15ca0be12c2c133ff40516f79e6fdf7f
> 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1113,6 +1113,9 @@ int tcp_md5_do_add(struct sock *sk, const union
> tcp_md5_addr *addr,
>if (key) {
>/* Pre-existing entry - just update that one. */
>memcpy(key->key, newkey, newkeylen);
> +
> + smp_wmb(); /* pairs with smp_rmb() in tcp_md5_hash_key() */
> +
>key->keylen = newkeylen;
>return 0;
>}
This doesn't make sense. Your smp_rmb only guarantees that you
see a version of key->key that's newer than keylen. What if the
key got changed twice? You could still read more than what's in
the key (if that's what you're trying to protect against).
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: build failures after merge of the vfs tree
On Tue, Jun 30, 2020 at 11:58:57AM +1000, Stephen Rothwell wrote: > > > > OK, here's a patch for both of these together: > > > > > > diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c > > > index 32b7a30b2485..eb382ceaa116 100644 > > > --- a/arch/s390/lib/test_unwind.c > > > +++ b/arch/s390/lib/test_unwind.c > > > @@ -9,6 +9,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c > > > b/drivers/remoteproc/qcom_q6v5_mss.c > > > index feb70283b6a2..903b2bb97e12 100644 > > > --- a/drivers/remoteproc/qcom_q6v5_mss.c > > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > > > @@ -26,6 +26,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #include "remoteproc_internal.h" > > > #include "qcom_common.h" > > > > I have applied those 2 by hand for today. > > I am still applying the above patch. Hi Al: Could you please fold these changes into your tree? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: missing retval check of call_netdevice_notifiers in dev_change_net_namespace
On Mon, Jun 22, 2020 at 12:43:53PM -0500, Eric W. Biederman wrote:
>
> Adding Herbert Xu who added support for failing notifications in
> fcc5a03ac425 ("[NET]: Allow netdev REGISTER/CHANGENAME events to fail").
>
> He might have some insight but 2007 was a long time ago.
https://lists.openwall.net/netdev/2007/07/26/61
AFAICS this is still needed.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: build failures after merge of the vfs tree
On Wed, Jun 17, 2020 at 05:31:02PM +1000, Stephen Rothwell wrote: > > > > > > Presumably another include needed: > > > > > > arch/s390/lib/test_unwind.c:49:2: error: implicit declaration of function > > > 'kmalloc' [-Werror=implicit-function-declaration] > > > arch/s390/lib/test_unwind.c:99:2: error: implicit declaration of function > > > 'kfree' [-Werror=implicit-function-declaration] > > And more (these are coming from other's builds): > > drivers/remoteproc/qcom_q6v5_mss.c:772:3: error: implicit declaration of > function 'kfree' [-Werror,-Wimplicit-function-declaration] > drivers/remoteproc/qcom_q6v5_mss.c:808:2: error: implicit declaration of > function 'kfree' [-Werror,-Wimplicit-function-declaration] > drivers/remoteproc/qcom_q6v5_mss.c:1195:2: error: implicit declaration of > function 'kfree' [-Werror,-Wimplicit-function-declaration] > > They may have other causes as they are full linux-next builds (not just > after the merge of the vfs tree), but the timing is suspicious. OK, here's a patch for both of these together: diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c index 32b7a30b2485..eb382ceaa116 100644 --- a/arch/s390/lib/test_unwind.c +++ b/arch/s390/lib/test_unwind.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index feb70283b6a2..903b2bb97e12 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -26,6 +26,7 @@ #include #include #include +#include #include "remoteproc_internal.h" #include "qcom_common.h" -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: build failures after merge of the vfs tree
On Wed, Jun 17, 2020 at 04:57:15PM +1000, Stephen Rothwell wrote: > > Presumably another include needed: > > arch/s390/lib/test_unwind.c:49:2: error: implicit declaration of function > 'kmalloc' [-Werror=implicit-function-declaration] > arch/s390/lib/test_unwind.c:99:2: error: implicit declaration of function > 'kfree' [-Werror=implicit-function-declaration] Hi Stephen: It's not clear how this file manages to include linux/uio.h but here is a patch for it anyway: diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c index 32b7a30b2485..eb382ceaa116 100644 --- a/arch/s390/lib/test_unwind.c +++ b/arch/s390/lib/test_unwind.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: build failures after merge of the vfs tree
On Tue, Jun 16, 2020 at 04:38:49AM +0100, Al Viro wrote: > > Folded and pushed Thanks Al. Here's another one that I just got, could you add this one too? diff --git a/drivers/mtd/nand/raw/cadence-nand-controller.c b/drivers/mtd/nand/raw/cadence-nand-controller.c index c405722adfe1..c4f273e2fe78 100644 --- a/drivers/mtd/nand/raw/cadence-nand-controller.c +++ b/drivers/mtd/nand/raw/cadence-nand-controller.c @@ -17,6 +17,7 @@ #include #include #include +#include /* * HPNFC can work in 3 modes: Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: build failures after merge of the vfs tree
On Tue, Jun 16, 2020 at 10:34:40AM +1000, Stephen Rothwell wrote:
> [Just adding Herbert to cc]
>
> On Tue, 16 Jun 2020 10:33:30 +1000 Stephen Rothwell
> wrote:
> >
> > Hi all,
> >
> > After merging the vfs tree, today's linux-next build (x86_64 allmodconfig)
> > failed like this:
Thanks Stephen, here is an incremental patch to fix these up.
---8<---
Because linux/uio.h included crypto/hash.h a number of header
files that should have been included weren't. This patch adds
linux/slab.h where kmalloc/kfree are used, as well as a forward
declaration in linux/socket.h for struct file.
Reported-by: Stephen Rothwell
Fixes: f0187db056dc ("iov_iter: Move unnecessary inclusion of...")
Signed-off-by: Herbert Xu
diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
index 67087dbe2f9f..962b6e05287b 100644
--- a/drivers/dma/st_fdma.c
+++ b/drivers/dma/st_fdma.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include "st_fdma.h"
diff --git a/drivers/dma/uniphier-xdmac.c b/drivers/dma/uniphier-xdmac.c
index 7b2f8a8c2d31..16b19654873d 100644
--- a/drivers/dma/uniphier-xdmac.c
+++ b/drivers/dma/uniphier-xdmac.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include "dmaengine.h"
#include "virt-dma.h"
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 04d2bc97f497..e9cb30d8cbfb 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -10,6 +10,7 @@
#include /* __user */
#include
+struct file;
struct pid;
struct cred;
struct socket;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v3 PATCH] iov_iter: Move unnecessary inclusion of crypto/hash.h
The header file linux/uio.h includes crypto/hash.h which pulls in
most of the Crypto API. Since linux/uio.h is used throughout the
kernel this means that every tiny bit of change to the Crypto API
causes the entire kernel to get rebuilt.
This patch fixes this by moving it into lib/iov_iter.c instead
where it is actually used.
This patch also fixes the ifdef to use CRYPTO_HASH instead of just
CRYPTO which does not guarantee the existence of ahash.
Unfortunately a number of drivers were relying on linux/uio.h to
provide access to linux/slab.h. This patch adds inclusions of
linux/slab.h as detected by build failures.
Also skbuff.h was relying on this to provide a declaration for
ahash_request. This patch adds a forward declaration instead.
Signed-off-by: Herbert Xu
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
index 6d0bec947636..e237d6038407 100644
--- a/drivers/dma/sf-pdma/sf-pdma.c
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include "sf-pdma.h"
diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index d39307f060bd..5a984df0e95e 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -4,6 +4,7 @@
#include
#include
#include
+#include
#include
static struct class *uacce_class;
diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c
index 17ad3b8698e1..aa2e3fe19c0f 100644
--- a/drivers/soc/qcom/pdr_interface.c
+++ b/drivers/soc/qcom/pdr_interface.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 320d1062068d..d1e03b8cb6bb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3,6 +3,7 @@
* Copyright (C) 2007 Oracle. All rights reserved.
*/
+#include
#include
#include
#include
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 9576fd8158d7..3835a8a8e9ea 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -7,7 +7,6 @@
#include
#include
-#include
#include
struct page;
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 51595bf3af85..2830daf46c73 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
+#include
#include
#include
#include
@@ -1566,7 +1567,7 @@ EXPORT_SYMBOL(csum_and_copy_to_iter);
size_t hash_and_copy_to_iter(const void *addr, size_t bytes, void *hashp,
struct iov_iter *i)
{
-#ifdef CONFIG_CRYPTO
+#ifdef CONFIG_CRYPTO_HASH
struct ahash_request *hash = hashp;
struct scatterlist sg;
size_t copied;
diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
index a4fe6060b960..a3ae8778f6a9 100644
--- a/drivers/mtd/mtdpstore.c
+++ b/drivers/mtd/mtdpstore.c
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
static struct mtdpstore_context {
int index;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3a2ac7072dbb..36df5998d23c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -238,6 +238,7 @@
SKB_DATA_ALIGN(sizeof(struct sk_buff)) + \
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
+struct ahash_request;
struct net_device;
struct scatterlist;
struct pipe_inode_info;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
On Thu, Jun 11, 2020 at 11:38:39AM +0530, Ayush Sawal wrote: > > Sorry for this hack, Our problem was when ipsec is under use and device is > dettached, then chcr_unregister_alg() > is called which unregisters the algorithms, but as ipsec is established the > cra_refcnt is not 1 and it gives a kernel bug. > So i put a check of cra_refcnt there, taking the reference of a crypto > driver "marvell/octeontx/otx_cptvf_algs.c" > is_any_alg_used(void) function where cra_refcnt is checked before > unregistering the algorithms. I understand. The question is how do you want to deal with the exception. IOW do you want to leave the algorithm still registered? If you can keep the algorithm registered you might as well never unregister it in the first place. If it has to go then this code path must wait for the users to disappear first. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal
On Wed, Jun 10, 2020 at 05:05:43PM -0700, David Miller wrote: > > Maybe we can start handling these changes via the crypto tree at some > point? Yes that's good point Dave. How about we push changes for chcr_algo via the crypto tree and the rest via netdev? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
On Wed, Jun 10, 2020 at 02:54:32AM +0530, Ayush Sawal wrote:
> This patch puts a check for algorithm unregister, to avoid removal of
> driver if the algorithm is under use.
>
> Signed-off-by: Ayush Sawal
> ---
> drivers/crypto/chelsio/chcr_algo.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/chelsio/chcr_algo.c
> b/drivers/crypto/chelsio/chcr_algo.c
> index f8b55137cf7d..4c2553672b6f 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -4391,22 +4391,32 @@ static int chcr_unregister_alg(void)
> for (i = 0; i < ARRAY_SIZE(driver_algs); i++) {
> switch (driver_algs[i].type & CRYPTO_ALG_TYPE_MASK) {
> case CRYPTO_ALG_TYPE_SKCIPHER:
> - if (driver_algs[i].is_registered)
> + if (driver_algs[i].is_registered && refcount_read(
> + &driver_algs[i].alg.skcipher.base.cra_refcnt)
> + == 1) {
> crypto_unregister_skcipher(
> &driver_algs[i].alg.skcipher);
> + driver_algs[i].is_registered = 0;
> + }
This is wrong. cra_refcnt must not be used directly by drivers.
Normally driver unregister is stopped by the module reference
count. This is not the case for your driver because of the existence
of a path of unregistration that is not tied to module removal.
To support that properly, we need to add code to the Crypto API
to handle this, as opposed to adding hacks to the driver.
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net v3 2/3] esp: select CRYPTO_SEQIV
On Tue, Jun 09, 2020 at 05:54:01PM -0700, Eric Biggers wrote:
> From: Eric Biggers
>
> Commit f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV") made
> CRYPTO_CTR stop selecting CRYPTO_SEQIV. This breaks IPsec for most
> users since GCM and several other encryption algorithms require "seqiv"
> -- and RFC 8221 lists AES-GCM as "MUST" be implemented.
>
> Just make XFRM_ESP select CRYPTO_SEQIV.
>
> Fixes: f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV") made
> Cc: Corentin Labbe
> Cc: Greg Kroah-Hartman
> Cc: Herbert Xu
> Cc: Steffen Klassert
> Signed-off-by: Eric Biggers
> ---
> net/xfrm/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Herbert Xu
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net v3 3/3] esp, ah: modernize the crypto algorithm selections
On Tue, Jun 09, 2020 at 05:54:02PM -0700, Eric Biggers wrote: > From: Eric Biggers > > The crypto algorithms selected by the ESP and AH kconfig options are > out-of-date with the guidance of RFC 8221, which lists the legacy > algorithms MD5 and DES as "MUST NOT" be implemented, and some more > modern algorithms like AES-GCM and HMAC-SHA256 as "MUST" be implemented. > But the options select the legacy algorithms, not the modern ones. > > Therefore, modify these options to select the MUST algorithms -- > and *only* the MUST algorithms. > > Also improve the help text. > > Suggested-by: Herbert Xu > Suggested-by: Steffen Klassert > Cc: Corentin Labbe > Cc: Greg Kroah-Hartman > Signed-off-by: Eric Biggers > --- > net/ipv4/Kconfig | 21 +++-- > net/ipv6/Kconfig | 21 +++-- > net/xfrm/Kconfig | 15 +++++-- > 3 files changed, 47 insertions(+), 10 deletions(-) Acked-by: Herbert Xu -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net v3 1/3] esp, ah: consolidate the crypto algorithm selections
On Tue, Jun 09, 2020 at 05:54:00PM -0700, Eric Biggers wrote: > From: Eric Biggers > > Instead of duplicating the algorithm selections between INET_AH and > INET6_AH and between INET_ESP and INET6_ESP, create new tristates > XFRM_AH and XFRM_ESP that do the algorithm selections, and make these be > selected by the corresponding INET* options. > > Suggested-by: Herbert Xu > Cc: Corentin Labbe > Cc: Greg Kroah-Hartman > Cc: Steffen Klassert > Signed-off-by: Eric Biggers > --- > net/ipv4/Kconfig | 16 ++-- > net/ipv6/Kconfig | 16 ++-- > net/xfrm/Kconfig | 20 > 3 files changed, 24 insertions(+), 28 deletions(-) Acked-by: Herbert Xu -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net v2] esp: select CRYPTO_SEQIV when useful
On Fri, Jun 05, 2020 at 11:00:23AM -0700, Eric Biggers wrote: > > Oops, this doesn't actually work: > > scripts/kconfig/conf --olddefconfig Kconfig > crypto/Kconfig:1799:error: recursive dependency detected! > crypto/Kconfig:1799: symbol CRYPTO_DRBG_MENU is selected by > CRYPTO_RNG_DEFAULT > crypto/Kconfig:83:symbol CRYPTO_RNG_DEFAULT is selected by CRYPTO_SEQIV > crypto/Kconfig:330: symbol CRYPTO_SEQIV is selected by CRYPTO_CTR > crypto/Kconfig:370: symbol CRYPTO_CTR is selected by CRYPTO_DRBG_CTR > crypto/Kconfig:1819: symbol CRYPTO_DRBG_CTR depends on CRYPTO_DRBG_MENU > For a resolution refer to Documentation/kbuild/kconfig-language.rst > subsection "Kconfig recursive dependency limitations" > > I guess we need to go with v1 (which just had 'select CRYPTO_SEQIV'), > or else make users explicitly select CRYPTO_SEQIV? OK, let's just go with the unconditional select on SEQIV since Steffen recommended RFC8221 which lists GCM and CBC as MUST and GCM requires SEQIV to work. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net] esp: select CRYPTO_SEQIV
On Thu, Jun 04, 2020 at 10:09:10PM -0700, Eric Biggers wrote:
>
> There's also a case where "seqiv" is used without counter mode:
>
> net/xfrm/xfrm_algo.c:
>
> {
> .name = "rfc7539esp(chacha20,poly1305)",
So
select CRYPTO_SEQIV if CRYPTO_CTR || CRYPTO_CHACHA20POLY1305
and if the list gets too long we could create another symbol that is
selected by the algorithms, say CRYPTO_SEQIV_ESP which could then
be used as the condition:
config CRYPTO_SEQIV_ESP
bool
config CRYPTO_CTR
select CRYPTO_SEQIV_ESP
config INET_ESP
select CRYPTO_SEQIV if CRYPTO_SEQIV_ESP
> FWIW, we make CONFIG_FS_ENCRYPTION select only the algorithms that we consider
> the "default", and any "non-default" algorithms need to be explicitly enabled.
>
> Is something similar going on here with INET_ESP and INET_ESP6? Should
> "seqiv"
> be considered a "default" for IPsec?
The default with IPsec is up to the user-space IPsec manager,
e.g., libreswan. So the kernel has no idea what the default
is. Also, unlike filesystems IPsec is about interoperability
so the default is actually a list of algorithms.
If you were using libreswan then top of the list is gcm(aes),
followed by aes(cbc)+sha256, and then aes(cbc)+sha1.
Incidentally we should probably prune the INET_ESP select list.
At least MD5/SHA1/DES shouldn't be on it. We probably should
add AES, SHA256 and GCM to the list.
Another potential improvement is to merge the two select lists
between ESP and ESP6. Perhaps move them to a new tristate say
XFRM_ESP that would then be selected by ESP and ESP6.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net] esp: select CRYPTO_SEQIV
On Fri, Jun 05, 2020 at 10:28:58AM +1000, Herbert Xu wrote: > > Hmm, the selection list doesn't include CTR so just adding SEQIV > per se makes no sense. I'm not certain that we really want to > include every algorithm under the sun. Steffen, what do you think? Or how about select CRYPTO_SEQIV if CRYPTO_CTR That would make more sense. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net] esp: select CRYPTO_SEQIV
On Thu, Jun 04, 2020 at 12:23:22PM -0700, Eric Biggers wrote:
> From: Eric Biggers
>
> Since CRYPTO_CTR no longer selects CRYPTO_SEQIV, it should be selected
> by INET_ESP and INET6_ESP -- similar to CRYPTO_ECHAINIV.
>
> Fixes: f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV")
> Cc: Corentin Labbe
> Cc: Greg Kroah-Hartman
> Cc: Herbert Xu
> Cc: Steffen Klassert
> Signed-off-by: Eric Biggers
> ---
> net/ipv4/Kconfig | 1 +
> net/ipv6/Kconfig | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> index 23ba5045e3d3..d393e8132aa1 100644
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -361,6 +361,7 @@ config INET_ESP
> select CRYPTO_SHA1
> select CRYPTO_DES
> select CRYPTO_ECHAINIV
> + select CRYPTO_SEQIV
> ---help---
> Support for IPsec ESP.
Hmm, the selection list doesn't include CTR so just adding SEQIV
per se makes no sense. I'm not certain that we really want to
include every algorithm under the sun. Steffen, what do you think?
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] rhashtable: Drop raw RCU deref in nested_table_free
This patch replaces some unnecessary uses of rcu_dereference_raw
in the rhashtable code with rcu_dereference_protected.
The top-level nested table entry is only marked as RCU because it
shares the same type as the tree entries underneath it. So it
doesn't need any RCU protection.
We also don't need RCU protection when we're freeing a nested RCU
table because by this stage we've long passed a memory barrier
when anyone could change the nested table.
Signed-off-by: Herbert Xu
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index bdb7e4cadf05..9f6890aedd1a 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -63,13 +63,22 @@ EXPORT_SYMBOL_GPL(lockdep_rht_bucket_is_held);
#define ASSERT_RHT_MUTEX(HT)
#endif
+static inline union nested_table *nested_table_top(
+ const struct bucket_table *tbl)
+{
+ /* The top-level bucket entry does not need RCU protection
+* because it's set at the same time as tbl->nest.
+*/
+ return (void *)rcu_dereference_protected(tbl->buckets[0], 1);
+}
+
static void nested_table_free(union nested_table *ntbl, unsigned int size)
{
const unsigned int shift = PAGE_SHIFT - ilog2(sizeof(void *));
const unsigned int len = 1 << shift;
unsigned int i;
- ntbl = rcu_dereference_raw(ntbl->table);
+ ntbl = rcu_dereference_protected(ntbl->table, 1);
if (!ntbl)
return;
@@ -89,7 +98,7 @@ static void nested_bucket_table_free(const struct
bucket_table *tbl)
union nested_table *ntbl;
unsigned int i;
- ntbl = (union nested_table *)rcu_dereference_raw(tbl->buckets[0]);
+ ntbl = nested_table_top(tbl);
for (i = 0; i < len; i++)
nested_table_free(ntbl + i, size);
@@ -1173,7 +1182,7 @@ struct rhash_lock_head **__rht_bucket_nested(const struct
bucket_table *tbl,
unsigned int subhash = hash;
union nested_table *ntbl;
- ntbl = (union nested_table *)rcu_dereference_raw(tbl->buckets[0]);
+ ntbl = nested_table_top(tbl);
ntbl = rht_dereference_bucket_rcu(ntbl[index].table, tbl, hash);
subhash >>= tbl->nest;
@@ -1213,7 +1222,7 @@ struct rhash_lock_head **rht_bucket_nested_insert(struct
rhashtable *ht,
unsigned int size = tbl->size >> tbl->nest;
union nested_table *ntbl;
- ntbl = (union nested_table *)rcu_dereference_raw(tbl->buckets[0]);
+ ntbl = nested_table_top(tbl);
hash >>= tbl->nest;
ntbl = nested_table_alloc(ht, &ntbl[index].table,
size <= (1 << shift));
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2] net: ipv4: move tcp_fastopen server side code to SipHash library
On Fri, Jun 14, 2019 at 04:01:22PM +0200, Ard Biesheuvel wrote: > Using a bare block cipher in non-crypto code is almost always a bad idea, > not only for security reasons (and we've seen some examples of this in > the kernel in the past), but also for performance reasons. > > In the TCP fastopen case, we call into the bare AES block cipher one or > two times (depending on whether the connection is IPv4 or IPv6). On most > systems, this results in a call chain such as > > crypto_cipher_encrypt_one(ctx, dst, src) > crypto_cipher_crt(tfm)->cit_encrypt_one(crypto_cipher_tfm(tfm), ...); > aesni_encrypt > kernel_fpu_begin(); > aesni_enc(ctx, dst, src); // asm routine > kernel_fpu_end(); > > It is highly unlikely that the use of special AES instructions has a > benefit in this case, especially since we are doing the above twice > for IPv6 connections, instead of using a transform which can process > the entire input in one go. > > We could switch to the cbcmac(aes) shash, which would at least get > rid of the duplicated overhead in *some* cases (i.e., today, only > arm64 has an accelerated implementation of cbcmac(aes), while x86 will > end up using the generic cbcmac template wrapping the AES-NI cipher, > which basically ends up doing exactly the above). However, in the given > context, it makes more sense to use a light-weight MAC algorithm that > is more suitable for the purpose at hand, such as SipHash. > > Since the output size of SipHash already matches our chosen value for > TCP_FASTOPEN_COOKIE_SIZE, and given that it accepts arbitrary input > sizes, this greatly simplifies the code as well. > > Signed-off-by: Ard Biesheuvel > --- > v2: rebase onto net-next > reverse order of operands in BUILD_BUG_ON() comparison expression > > include/linux/tcp.h | 7 +- > include/net/tcp.h | 10 +- > net/ipv4/tcp_fastopen.c | 97 +++- > 3 files changed, 36 insertions(+), 78 deletions(-) You should also revert commit 798b2cbf9227 in your patch: commit 798b2cbf9227b1bd7d37ae9af4d9c750e6f4de9c Author: David S. Miller Date: Tue Sep 4 14:20:14 2012 -0400 net: Add INET dependency on aes crypto for the sake of TCP fastopen. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH ipsec] xfrm: fix sa selector validation
On Fri, Jun 14, 2019 at 11:13:55AM +0200, Nicolas Dichtel wrote:
> After commit b38ff4075a80, the following command does not work anymore:
> $ ip xfrm state add src 10.125.0.2 dst 10.125.0.1 proto esp spi 34 reqid 1 \
> mode tunnel enc 'cbc(aes)' 0xb0abdba8b782ad9d364ec81e3a7d82a1 auth-trunc \
> 'hmac(sha1)' 0xe26609ebd00acb6a4d51fca13e49ea78a72c73e6 96 flag align4
>
> In fact, the selector is not mandatory, allow the user to provide an empty
> selector.
>
> Fixes: b38ff4075a80 ("xfrm: Fix xfrm sel prefix length validation")
> CC: Anirudh Gupta
> Signed-off-by: Nicolas Dichtel
Acked-by: Herbert Xu
Sorry for not catching this!
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] inet: frags: Remove unnecessary smp_store_release/READ_ONCE
On Fri, May 31, 2019 at 10:24:08AM +0200, Dmitry Vyukov wrote: > > OK, let's call it barrier. But we need more than a barrier here then. READ_ONCE/WRITE_ONCE is not some magical dust that you sprinkle around in your code to make it work without locks. You need to understand exactly why you need them and why the code would be buggy if you don't use them. In this case the code doesn't need them because an implicit barrier() (which is *stronger* than READ_ONCE/WRITE_ONCE) already exists in both places. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] inet: frags: Remove unnecessary smp_store_release/READ_ONCE
On Wed, May 29, 2019 at 07:43:51AM +0200, Dmitry Vyukov wrote: > > If fqdir->dead read/write are concurrent, then this still needs to be > READ_ONCE/WRITE_ONCE. Ordering is orthogonal to atomicity. No they do not. READ_ONCE/WRITE_ONCE are basically a more fine-tuned version of barrier(). In this case we already have an implicit barrier() call due to the memory barrier semantics so this is simply unnecessary. It's the same reason you don't need READ_ONCE/WRITE_ONCE when you do: CPU1CPU2 spin_lock shared_var = 1 spin_lock spin_unlock if (shared_var == 1) ... spin_unlock Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] inet: frags: Remove unnecessary smp_store_release/READ_ONCE
On Tue, May 28, 2019 at 06:31:00AM -0700, Eric Dumazet wrote:
>
> This smp_store_release() is a left over of the first version of the patch,
> where
> there was no rcu grace period enforcement.
>
> I do not believe there is harm letting this, but if you disagree
> please send a patch ;)
I see now that it is actually relying on the barrier/locking
semantics of call_rcu vs. rcu_read_lock. So the smp_store_release
and READ_ONCE are simply unnecessary and could be confusing to
future readers.
---8<---
The smp_store_release call in fqdir_exit cannot protect the setting
of fqdir->dead as claimed because its memory barrier is only
guaranteed to be one-way and the barrier precedes the setting of
fqdir->dead.
IOW it doesn't provide any barriers between fq->dir and the following
hash table destruction.
In fact, the code is safe anyway because call_rcu does provide both
the memory barrier as well as a guarantee that when the destruction
work starts executing all RCU readers will see the updated value for
fqdir->dead.
Therefore this patch removes the unnecessary smp_store_release call
as well as the corresponding READ_ONCE on the read-side in order to
not confuse future readers of this code. Comments have been added
in their places.
Signed-off-by: Herbert Xu
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 2b816f1ebbb4..35e9784fab4e 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -193,10 +193,12 @@ void fqdir_exit(struct fqdir *fqdir)
{
fqdir->high_thresh = 0; /* prevent creation of new frags */
- /* paired with READ_ONCE() in inet_frag_kill() :
-* We want to prevent rhashtable_remove_fast() calls
+ fqdir->dead = true;
+
+ /* call_rcu is supposed to provide memory barrier semantics,
+* separating the setting of fqdir->dead with the destruction
+* work. This implicit barrier is paired with inet_frag_kill().
*/
- smp_store_release(&fqdir->dead, true);
INIT_RCU_WORK(&fqdir->destroy_rwork, fqdir_rwork_fn);
queue_rcu_work(system_wq, &fqdir->destroy_rwork);
@@ -214,10 +216,12 @@ void inet_frag_kill(struct inet_frag_queue *fq)
fq->flags |= INET_FRAG_COMPLETE;
rcu_read_lock();
- /* This READ_ONCE() is paired with smp_store_release()
-* in inet_frags_exit_net().
+ /* The RCU read lock provides a memory barrier
+* guaranteeing that if fqdir->dead is false then
+* the hash table destruction will not start until
+* after we unlock. Paired with inet_frags_exit_net().
*/
- if (!READ_ONCE(fqdir->dead)) {
+ if (!fqdir->dead) {
rhashtable_remove_fast(&fqdir->rhashtable, &fq->node,
fqdir->f->rhash_params);
refcount_dec(&fq->refcnt);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] rhashtable: Add rht_ptr_rcu and improve rht_ptr
This patch moves common code between rht_ptr and rht_ptr_exclusive
into __rht_ptr. It also adds a new helper rht_ptr_rcu exclusively
for the RCU case. This way rht_ptr becomes a lock-only construct
so we can use the lighter rcu_dereference_protected primitive.
Signed-off-by: Herbert Xu
---
include/linux/rhashtable.h | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 9f8bc06d4136..beb9a9da1699 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -352,37 +352,38 @@ static inline void rht_unlock(struct bucket_table *tbl,
static inline struct rhash_head __rcu *__rht_ptr(
struct rhash_lock_head *const *bkt)
{
- return (struct rhash_head __rcu *)((unsigned long)*bkt & ~BIT(0));
+ return (struct rhash_head __rcu *)
+ ((unsigned long)*bkt & ~BIT(0) ?:
+(unsigned long)RHT_NULLS_MARKER(bkt));
}
/*
* Where 'bkt' is a bucket and might be locked:
- * rht_ptr() dereferences that pointer and clears the lock bit.
+ * rht_ptr_rcu() dereferences that pointer and clears the lock bit.
+ * rht_ptr() dereferences in a context where the bucket is locked.
* rht_ptr_exclusive() dereferences in a context where exclusive
*access is guaranteed, such as when destroying the table.
*/
+static inline struct rhash_head *rht_ptr_rcu(
+ struct rhash_lock_head *const *bkt)
+{
+ struct rhash_head __rcu *p = __rht_ptr(bkt);
+
+ return rcu_dereference(p);
+}
+
static inline struct rhash_head *rht_ptr(
struct rhash_lock_head *const *bkt,
struct bucket_table *tbl,
unsigned int hash)
{
- struct rhash_head __rcu *p = __rht_ptr(bkt);
-
- if (!p)
- return RHT_NULLS_MARKER(bkt);
-
- return rht_dereference_bucket_rcu(p, tbl, hash);
+ return rht_dereference_bucket(__rht_ptr(bkt), tbl, hash);
}
static inline struct rhash_head *rht_ptr_exclusive(
struct rhash_lock_head *const *bkt)
{
- struct rhash_head __rcu *p = __rht_ptr(bkt);
-
- if (!p)
- return RHT_NULLS_MARKER(bkt);
-
- return rcu_dereference_protected(p, 1);
+ return rcu_dereference_protected(__rht_ptr(bkt), 1);
}
static inline void rht_assign_locked(struct rhash_lock_head **bkt,
@@ -509,7 +510,7 @@ static inline void rht_assign_unlock(struct bucket_table
*tbl,
*/
#define rht_for_each_rcu(pos, tbl, hash) \
for (({barrier(); }), \
-pos = rht_ptr(rht_bucket(tbl, hash), tbl, hash); \
+pos = rht_ptr_rcu(rht_bucket(tbl, hash)); \
!rht_is_a_nulls(pos); \
pos = rcu_dereference_raw(pos->next))
@@ -546,8 +547,7 @@ static inline void rht_assign_unlock(struct bucket_table
*tbl,
*/
#define rht_for_each_entry_rcu(tpos, pos, tbl, hash, member) \
rht_for_each_entry_rcu_from(tpos, pos, \
- rht_ptr(rht_bucket(tbl, hash), \
- tbl, hash),\
+ rht_ptr_rcu(rht_bucket(tbl, hash)),\
tbl, hash, member)
/**
@@ -603,7 +603,7 @@ static inline struct rhash_head *__rhashtable_lookup(
hash = rht_key_hashfn(ht, tbl, key, params);
bkt = rht_bucket(tbl, hash);
do {
- rht_for_each_rcu_from(he, rht_ptr(bkt, tbl, hash), tbl, hash) {
+ rht_for_each_rcu_from(he, rht_ptr_rcu(bkt), tbl, hash) {
if (params.obj_cmpfn ?
params.obj_cmpfn(&arg, rht_obj(ht, he)) :
rhashtable_compare(&arg, rht_obj(ht, he)))
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net-next 11/11] inet: frags: rework rhashtable dismantle
Hi Eric:
Eric Dumazet wrote:
>
> +void fqdir_exit(struct fqdir *fqdir)
> +{
> + fqdir->high_thresh = 0; /* prevent creation of new frags */
> +
> + /* paired with READ_ONCE() in inet_frag_kill() :
> +* We want to prevent rhashtable_remove_fast() calls
> +*/
> + smp_store_release(&fqdir->dead, true);
> +
> + INIT_RCU_WORK(&fqdir->destroy_rwork, fqdir_rwork_fn);
> + queue_rcu_work(system_wq, &fqdir->destroy_rwork);
> +
> +}
What is the smp_store_release supposed to protect here? If it's
meant to separate the setting of dead and the subsequent destruction
work then it doesn't work because the barrier only protects the code
preceding it, not after.
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net] xfrm: Fix xfrm sel prefix length validation
On Tue, May 21, 2019 at 08:59:47PM +0530, Anirudh Gupta wrote:
> Family of src/dst can be different from family of selector src/dst.
> Use xfrm selector family to validate address prefix length,
> while verifying new sa from userspace.
>
> Validated patch with this command:
> ip xfrm state add src 1.1.6.1 dst 1.1.6.2 proto esp spi 4260196 \
> reqid 20004 mode tunnel aead "rfc4106(gcm(aes))" \
> 0x01640001 128 \
> sel src 1011:1:4::2/128 sel dst 1021:1:4::2/128 dev Port5
>
> Fixes: 07bf7908950a ("xfrm: Validate address prefix lengths in the xfrm
> selector.")
> Signed-off-by: Anirudh Gupta
Acked-by: Herbert Xu
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 2/2] rhashtable: Fix cmpxchg RCU warnings
On Thu, May 16, 2019 at 09:20:36AM +, David Laight wrote: > > I presume these casts remove an 'rcu' marker on the variable. > Is there a way of marking such casts as 'for sparse only' so > that the compiler does proper type checking. > (Clearly this isn't that relevant here as the cast could be (void **).) > > Hmmm something should be checking that the type of the argument > to cmpxchg is 'pointer to "something the size of a pointer"' > Adding any kind of cast subverts that test. If we were adding this as an RCU primitive then yes that what it should do. But it isn't relevant to this patch. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH 2/2] rhashtable: Fix cmpxchg RCU warnings
As cmpxchg is a non-RCU mechanism it will cause sparse warnings when we use it for RCU. This patch adds explicit casts to silence those warnings. This should probably be moved to RCU itself in future. Signed-off-by: Herbert Xu --- lib/rhashtable.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 7708699a5b96..935ec80f213f 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -131,7 +131,7 @@ static union nested_table *nested_table_alloc(struct rhashtable *ht, INIT_RHT_NULLS_HEAD(ntbl[i].bucket); } - if (cmpxchg(prev, NULL, ntbl) == NULL) + if (cmpxchg((union nested_table **)prev, NULL, ntbl) == NULL) return ntbl; /* Raced with another thread. */ kfree(ntbl); @@ -296,7 +296,8 @@ static int rhashtable_rehash_attach(struct rhashtable *ht, * rcu_assign_pointer(). */ - if (cmpxchg(&old_tbl->future_tbl, NULL, new_tbl) != NULL) + if (cmpxchg((struct bucket_table **)&old_tbl->future_tbl, NULL, + new_tbl) != NULL) return -EEXIST; return 0;
[PATCH 1/2] rhashtable: Remove RCU marking from rhash_lock_head
The opaque type rhash_lock_head should not be marked with __rcu
because it can never be dereferenced. We should apply the RCU
marking when we turn it into a pointer which can be dereferenced.
This patch does exactly that. This fixes a number of sparse
warnings as well as getting rid of some unnecessary RCU checking.
Signed-off-by: Herbert Xu
---
include/linux/rhashtable.h | 58 -
lib/rhashtable.c | 28 ++---
2 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index f7714d3b46bd..9f8bc06d4136 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -84,7 +84,7 @@ struct bucket_table {
struct lockdep_map dep_map;
- struct rhash_lock_head __rcu *buckets[] cacheline_aligned_in_smp;
+ struct rhash_lock_head *buckets[] cacheline_aligned_in_smp;
};
/*
@@ -261,13 +261,13 @@ void rhashtable_free_and_destroy(struct rhashtable *ht,
void *arg);
void rhashtable_destroy(struct rhashtable *ht);
-struct rhash_lock_head __rcu **rht_bucket_nested(const struct bucket_table
*tbl,
-unsigned int hash);
-struct rhash_lock_head __rcu **__rht_bucket_nested(const struct bucket_table
*tbl,
- unsigned int hash);
-struct rhash_lock_head __rcu **rht_bucket_nested_insert(struct rhashtable *ht,
- struct bucket_table
*tbl,
- unsigned int hash);
+struct rhash_lock_head **rht_bucket_nested(const struct bucket_table *tbl,
+ unsigned int hash);
+struct rhash_lock_head **__rht_bucket_nested(const struct bucket_table *tbl,
+unsigned int hash);
+struct rhash_lock_head **rht_bucket_nested_insert(struct rhashtable *ht,
+ struct bucket_table *tbl,
+ unsigned int hash);
#define rht_dereference(p, ht) \
rcu_dereference_protected(p, lockdep_rht_mutex_is_held(ht))
@@ -284,21 +284,21 @@ struct rhash_lock_head __rcu
**rht_bucket_nested_insert(struct rhashtable *ht,
#define rht_entry(tpos, pos, member) \
({ tpos = container_of(pos, typeof(*tpos), member); 1; })
-static inline struct rhash_lock_head __rcu *const *rht_bucket(
+static inline struct rhash_lock_head *const *rht_bucket(
const struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? rht_bucket_nested(tbl, hash) :
&tbl->buckets[hash];
}
-static inline struct rhash_lock_head __rcu **rht_bucket_var(
+static inline struct rhash_lock_head **rht_bucket_var(
struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? __rht_bucket_nested(tbl, hash) :
&tbl->buckets[hash];
}
-static inline struct rhash_lock_head __rcu **rht_bucket_insert(
+static inline struct rhash_lock_head **rht_bucket_insert(
struct rhashtable *ht, struct bucket_table *tbl, unsigned int hash)
{
return unlikely(tbl->nest) ? rht_bucket_nested_insert(ht, tbl, hash) :
@@ -349,6 +349,12 @@ static inline void rht_unlock(struct bucket_table *tbl,
local_bh_enable();
}
+static inline struct rhash_head __rcu *__rht_ptr(
+ struct rhash_lock_head *const *bkt)
+{
+ return (struct rhash_head __rcu *)((unsigned long)*bkt & ~BIT(0));
+}
+
/*
* Where 'bkt' is a bucket and might be locked:
* rht_ptr() dereferences that pointer and clears the lock bit.
@@ -356,30 +362,30 @@ static inline void rht_unlock(struct bucket_table *tbl,
*access is guaranteed, such as when destroying the table.
*/
static inline struct rhash_head *rht_ptr(
- struct rhash_lock_head __rcu * const *bkt,
+ struct rhash_lock_head *const *bkt,
struct bucket_table *tbl,
unsigned int hash)
{
- const struct rhash_lock_head *p =
- rht_dereference_bucket_rcu(*bkt, tbl, hash);
+ struct rhash_head __rcu *p = __rht_ptr(bkt);
- if unsigned long)p) & ~BIT(0)) == 0)
+ if (!p)
return RHT_NULLS_MARKER(bkt);
- return (void *)(((unsigned long)p) & ~BIT(0));
+
+ return rht_dereference_bucket_rcu(p, tbl, hash);
}
static inline struct rhash_head *rht_ptr_exclusive(
- struct rhash_lock_head __rcu * const *bkt)
+ struct rhash_lock_head *const *bkt)
{
- const struct rhash_lock_head *p =
- rcu_dereference_protected(*bkt, 1);
+ struct rhash_head __rcu *p = __rht_ptr(bkt);
if (!p)
return RHT_NULLS_MARKER(bkt);
- return (void *)(((
[PATCH 0/2] rhashtable: Fix sparse warnings
This patch series fixes all the sparse warnings. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer
On Wed, May 15, 2019 at 01:55:01PM -0700, Jakub Kicinski wrote:
> Since the bit_spin_lock() operations don't actually dereference
> the pointer, it's fine to forcefully drop the RCU annotation.
> This fixes 7 sparse warnings per include site.
>
> Fixes: 8f0db018006a ("rhashtable: use bit_spin_locks to protect hash bucket.")
> Signed-off-by: Jakub Kicinski
> Reviewed-by: Simon Horman
I don't think this is the right fix. We should remove the __rcu
marker from the opaque type rhash_lock_head since it cannot be
directly dereferenced.
I'm working on a fix to this.
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 1/4] rhashtable: use cmpxchg() in nested_table_alloc()
On Tue, Apr 02, 2019 at 10:07:45AM +1100, NeilBrown wrote: > nested_table_alloc() relies on the fact that there is > at most one spinlock allocated for every slot in the top > level nested table, so it is not possible for two threads > to try to allocate the same table at the same time. > > This assumption is a little fragile (it is not explicit) and is > unnecessary as cmpxchg() can be used instead. > > A future patch will replace the spinlocks by per-bucket bitlocks, > and then we won't be able to protect the slot pointer with a spinlock. > > So replace rcu_assign_pointer() with cmpxchg() - which has equivalent > barrier properties. > If it the cmp fails, free the table that was just allocated. > > Signed-off-by: NeilBrown Acked-by: Herbert Xu -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] ila: Fix rhashtable walker list corruption
ila_xlat_nl_cmd_flush uses rhashtable walkers allocated from the
stack but it never frees them. This corrupts the walker list of
the hash table.
This patch fixes it.
Reported-by: syzbot+dae72a112334aa65a...@syzkaller.appspotmail.com
Fixes: b6e71bdebb12 ("ila: Flush netlink command to clear xlat...")
Signed-off-by: Herbert Xu
diff --git a/net/ipv6/ila/ila_xlat.c b/net/ipv6/ila/ila_xlat.c
index 79d2e43c05c5..5fc1f4e0c0cf 100644
--- a/net/ipv6/ila/ila_xlat.c
+++ b/net/ipv6/ila/ila_xlat.c
@@ -417,6 +417,7 @@ int ila_xlat_nl_cmd_flush(struct sk_buff *skb, struct
genl_info *info)
done:
rhashtable_walk_stop(&iter);
+ rhashtable_walk_exit(&iter);
return ret;
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [Patch net v2] xfrm: clean up xfrm protocol checks
On Fri, Mar 22, 2019 at 04:26:19PM -0700, Cong Wang wrote:
> In commit 6a53b7593233 ("xfrm: check id proto in validate_tmpl()")
> I introduced a check for xfrm protocol, but according to Herbert
> IPSEC_PROTO_ANY should only be used as a wildcard for lookup, so
> it should be removed from validate_tmpl().
>
> And, IPSEC_PROTO_ANY is expected to only match 3 IPSec-specific
> protocols, this is why xfrm_state_flush() could still miss
> IPPROTO_ROUTING, which leads that those entries are left in
> net->xfrm.state_all before exit net. Fix this by replacing
> IPSEC_PROTO_ANY with zero.
>
> This patch also extracts the check from validate_tmpl() to
> xfrm_id_proto_valid() and uses it in parse_ipsecrequest().
> With this, no other protocols should be added into xfrm.
>
> Fixes: 6a53b7593233 ("xfrm: check id proto in validate_tmpl()")
> Reported-by: syzbot+0bf0519d6e0de1591...@syzkaller.appspotmail.com
> Cc: Steffen Klassert
> Cc: Herbert Xu
> Signed-off-by: Cong Wang
> ---
> include/net/xfrm.h | 17 +
> net/ipv6/xfrm6_tunnel.c | 2 +-
> net/key/af_key.c| 4 +++-
> net/xfrm/xfrm_state.c | 2 +-
> net/xfrm/xfrm_user.c| 14 +-----
> 5 files changed, 23 insertions(+), 16 deletions(-)
Acked-by: Herbert Xu
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [Patch net] xfrm: unify xfrm protocol checks
On Thu, Mar 21, 2019 at 09:06:05PM -0700, Cong Wang wrote: > > Good point. Replacing IPSEC_PROTO_ANY with zero should > work too, but on the other hand, id.proto is still never allowed to > be any other protocol than these 6 listed, no? It should never be IPSEC_PROTO_ANY since that's used as a wildcard. IOW if you're going to tighten up the check for the id.proto filed in an actual state, you should distinguish between the case of an ID that's used to add/modify a state vs. an ID that's be used to query a state. IPSEC_PROTO_ANY and zero should be denied in the first case and allowed in the second case. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [Patch net] xfrm: unify xfrm protocol checks
On Tue, Mar 19, 2019 at 01:42:53PM -0700, Cong Wang wrote: > > IIRC, it is Steffen who suggested to add IPPROTO_ROUTING/IPPROTO_DSTOPTS > back to commit 6a53b7593233. My xfrm knowledge is not enough to > figure out IPPROTO_ROUTING/IPPROTO_DSTOPTS. OK I dug into the history of xfrm_id_proto_match and this is definitely not right. The intention appears to be that IPSEC_PROTO_ANY should only match genuine IPsec protocols, i.e., AH/ESP/COMP, while the special value of zero will match everything. So I think what we should do is get rid of the validation function that you added in 6a5t3b7593233, and then change those internal functions which were incorrectly using IPSEC_PROTO_ANY to using zero instead. Does anybody still use IPPROTO_ROUTING/IPPROTO_DSTOPTS? It's always a pain when people come and add features and then don't shoulder the burden of maintaining them. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [Patch net] xfrm: unify xfrm protocol checks
On Mon, Mar 18, 2019 at 10:08:24PM -0700, Cong Wang wrote:
>
> +static inline bool xfrm_id_proto_valid(u8 proto)
> +{
> + switch (proto) {
> + case IPPROTO_AH:
> + case IPPROTO_ESP:
> + case IPPROTO_COMP:
> +#if IS_ENABLED(CONFIG_IPV6)
> + case IPPROTO_ROUTING:
> + case IPPROTO_DSTOPTS:
> +#endif
> + case IPSEC_PROTO_ANY:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static inline int xfrm_id_proto_match(u8 proto, u8 userproto)
> {
> return (!userproto || proto == userproto ||
> - (userproto == IPSEC_PROTO_ANY && (proto == IPPROTO_AH ||
> - proto == IPPROTO_ESP ||
> - proto == IPPROTO_COMP)));
> + (userproto == IPSEC_PROTO_ANY && xfrm_id_proto_valid(proto)));
> }
This does not look right. IPSEC_PROTO_ANY should only be allowed
in userproto and your patch is going to let it pass when it's in
proto. Whether IPPROTO_ROUTING/IPPROTO_DSTOPTS should be allowed
in this context is also not obvious.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
