* Re: [PATCH v6 18/18] crypto: Remove AHASH_REQUEST_ON_STACK
From: Kees Cook @ 2018-07-24 17:53 UTC (permalink / raw)
To: Joe Perches
Cc: Herbert Xu, Arnd Bergmann, Eric Biggers, Gustavo A. R. Silva,
Alasdair Kergon, Rabin Vincent, Tim Chen, Rafael J. Wysocki,
Pavel Machek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
X86 ML, Philipp Reisner, Lars Ellenberg, Jens Axboe,
Giovanni Cabiddu, Mike Snitzer, Paul Mackerras,
Greg Kroah-Hartman
In-Reply-To: <1bdc706ae86039c4ffcff39698251424d54af116.camel-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
On Tue, Jul 24, 2018 at 10:31 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Tue, 2018-07-24 at 09:49 -0700, Kees Cook wrote:
>> All users of AHASH_REQUEST_ON_STACK have been removed from the kernel, so
>> drop it entirely so no VLAs get reintroduced by future users.
>
> checkpatch has a test for that.
> It could now be removed as well.
> ---
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 34e4683de7a3..a3517334d661 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -796,7 +796,7 @@ our $declaration_macros = qr{(?x:
> (?:$Storage\s+)?(?:[A-Z_][A-Z0-9]*_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-9]+){1,6}\s*\(|
> (?:$Storage\s+)?[HLP]?LIST_HEAD\s*\(|
> (?:$Storage\s+)?${Type}\s+uninitialized_var\s*\(|
> - (?:SKCIPHER_REQUEST|SHASH_DESC|AHASH_REQUEST)_ON_STACK\s*\(
> + (?:SKCIPHER_REQUEST|SHASH_DESC)_ON_STACK\s*\(
> )};
Ah! Cool. I've added this now.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* [PATCH v6 06/18] crypto: qat: Remove VLA usage
From: Kees Cook @ 2018-07-24 16:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Kees Cook, Arnd Bergmann, Eric Biggers, Gustavo A. R. Silva,
Alasdair Kergon, Rabin Vincent, Tim Chen, Rafael J. Wysocki,
Pavel Machek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Philipp Reisner, Lars Ellenberg, Jens Axboe, Giovanni Cabiddu,
Mike Snitzer, Paul Mackerras, Greg Kroah-Hartman,
David Howells <dhowe
In-Reply-To: <20180724164936.37477-1-keescook@chromium.org>
In the quest to remove all stack VLA usage from the kernel[1], this uses
the new upper bound for the stack buffer. Also adds a sanity check.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/crypto/qat/qat_common/qat_algs.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
index 1138e41d6805..a28edf7b792f 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -153,8 +153,8 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
struct sha512_state sha512;
int block_size = crypto_shash_blocksize(ctx->hash_tfm);
int digest_size = crypto_shash_digestsize(ctx->hash_tfm);
- char ipad[block_size];
- char opad[block_size];
+ char ipad[MAX_ALGAPI_BLOCKSIZE];
+ char opad[MAX_ALGAPI_BLOCKSIZE];
__be32 *hash_state_out;
__be64 *hash512_state_out;
int i, offset;
@@ -164,6 +164,10 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
shash->tfm = ctx->hash_tfm;
shash->flags = 0x0;
+ if (WARN_ON(block_size > sizeof(ipad) ||
+ sizeof(ipad) != sizeof(opad)))
+ return -EINVAL;
+
if (auth_keylen > block_size) {
int ret = crypto_shash_digest(shash, auth_key,
auth_keylen, ipad);
--
2.17.1
^ permalink raw reply related
* [PATCH v6 07/18] crypto: shash: Remove VLA usage in unaligned hashing
From: Kees Cook @ 2018-07-24 16:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Kees Cook, Arnd Bergmann, Eric Biggers, Gustavo A. R. Silva,
Alasdair Kergon, Rabin Vincent, Tim Chen, Rafael J. Wysocki,
Pavel Machek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Philipp Reisner, Lars Ellenberg, Jens Axboe, Giovanni Cabiddu,
Mike Snitzer, Paul Mackerras, Greg Kroah-Hartman,
David Howells <dhowe
In-Reply-To: <20180724164936.37477-1-keescook@chromium.org>
In the quest to remove all stack VLA usage from the kernel[1], this uses
the newly defined max alignment to perform unaligned hashing to avoid
VLAs, and drops the helper function while adding sanity checks on the
resulting buffer sizes. Additionally, the __aligned_largest macro is
removed since this helper was the only user.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
crypto/shash.c | 27 ++++++++++++++++-----------
include/linux/compiler-gcc.h | 1 -
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/crypto/shash.c b/crypto/shash.c
index 86d76b5c626c..d21f04d70dce 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -73,13 +73,6 @@ int crypto_shash_setkey(struct crypto_shash *tfm, const u8 *key,
}
EXPORT_SYMBOL_GPL(crypto_shash_setkey);
-static inline unsigned int shash_align_buffer_size(unsigned len,
- unsigned long mask)
-{
- typedef u8 __aligned_largest u8_aligned;
- return len + (mask & ~(__alignof__(u8_aligned) - 1));
-}
-
static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
unsigned int len)
{
@@ -88,11 +81,17 @@ static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
unsigned long alignmask = crypto_shash_alignmask(tfm);
unsigned int unaligned_len = alignmask + 1 -
((unsigned long)data & alignmask);
- u8 ubuf[shash_align_buffer_size(unaligned_len, alignmask)]
- __aligned_largest;
+ /*
+ * We cannot count on __aligned() working for large values:
+ * https://patchwork.kernel.org/patch/9507697/
+ */
+ u8 ubuf[MAX_ALGAPI_ALIGNMASK * 2];
u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
int err;
+ if (WARN_ON(buf + unaligned_len > ubuf + sizeof(ubuf)))
+ return -EINVAL;
+
if (unaligned_len > len)
unaligned_len = len;
@@ -124,11 +123,17 @@ static int shash_final_unaligned(struct shash_desc *desc, u8 *out)
unsigned long alignmask = crypto_shash_alignmask(tfm);
struct shash_alg *shash = crypto_shash_alg(tfm);
unsigned int ds = crypto_shash_digestsize(tfm);
- u8 ubuf[shash_align_buffer_size(ds, alignmask)]
- __aligned_largest;
+ /*
+ * We cannot count on __aligned() working for large values:
+ * https://patchwork.kernel.org/patch/9507697/
+ */
+ u8 ubuf[MAX_ALGAPI_ALIGNMASK + HASH_MAX_DIGESTSIZE];
u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
int err;
+ if (WARN_ON(buf + ds > ubuf + sizeof(ubuf)))
+ return -EINVAL;
+
err = shash->final(desc, buf);
if (err)
goto out;
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index f1a7492a5cc8..1f1cdef36a82 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -125,7 +125,6 @@
*/
#define __pure __attribute__((pure))
#define __aligned(x) __attribute__((aligned(x)))
-#define __aligned_largest __attribute__((aligned))
#define __printf(a, b) __attribute__((format(printf, a, b)))
#define __scanf(a, b) __attribute__((format(scanf, a, b)))
#define __attribute_const__ __attribute__((__const__))
--
2.17.1
^ permalink raw reply related
* [PATCH v6 09/18] ppp: mppe: Remove VLA usage
From: Kees Cook @ 2018-07-24 16:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Kees Cook, Arnd Bergmann, Eric Biggers, Gustavo A. R. Silva,
Alasdair Kergon, Rabin Vincent, Tim Chen, Rafael J. Wysocki,
Pavel Machek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Philipp Reisner, Lars Ellenberg, Jens Axboe, Giovanni Cabiddu,
Mike Snitzer, Paul Mackerras, Greg Kroah-Hartman,
David Howells <dhowe
In-Reply-To: <20180724164936.37477-1-keescook@chromium.org>
In the quest to remove all stack VLA usage from the kernel[1], this
removes the discouraged use of AHASH_REQUEST_ON_STACK (and associated
VLA) by switching to shash directly and keeping the associated descriptor
allocated with the regular state on the heap.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ppp/ppp_mppe.c | 56 ++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 6c7fd98cb00a..a205750b431b 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -96,7 +96,7 @@ static inline void sha_pad_init(struct sha_pad *shapad)
*/
struct ppp_mppe_state {
struct crypto_skcipher *arc4;
- struct crypto_ahash *sha1;
+ struct shash_desc *sha1;
unsigned char *sha1_digest;
unsigned char master_key[MPPE_MAX_KEY_LEN];
unsigned char session_key[MPPE_MAX_KEY_LEN];
@@ -136,25 +136,16 @@ struct ppp_mppe_state {
*/
static void get_new_key_from_sha(struct ppp_mppe_state * state)
{
- AHASH_REQUEST_ON_STACK(req, state->sha1);
- struct scatterlist sg[4];
- unsigned int nbytes;
-
- sg_init_table(sg, 4);
-
- nbytes = setup_sg(&sg[0], state->master_key, state->keylen);
- nbytes += setup_sg(&sg[1], sha_pad->sha_pad1,
- sizeof(sha_pad->sha_pad1));
- nbytes += setup_sg(&sg[2], state->session_key, state->keylen);
- nbytes += setup_sg(&sg[3], sha_pad->sha_pad2,
- sizeof(sha_pad->sha_pad2));
-
- ahash_request_set_tfm(req, state->sha1);
- ahash_request_set_callback(req, 0, NULL, NULL);
- ahash_request_set_crypt(req, sg, state->sha1_digest, nbytes);
-
- crypto_ahash_digest(req);
- ahash_request_zero(req);
+ crypto_shash_init(state->sha1);
+ crypto_shash_update(state->sha1, state->master_key,
+ state->keylen);
+ crypto_shash_update(state->sha1, sha_pad->sha_pad1,
+ sizeof(sha_pad->sha_pad1));
+ crypto_shash_update(state->sha1, state->session_key,
+ state->keylen);
+ crypto_shash_update(state->sha1, sha_pad->sha_pad2,
+ sizeof(sha_pad->sha_pad2));
+ crypto_shash_final(state->sha1, state->sha1_digest);
}
/*
@@ -200,6 +191,7 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
static void *mppe_alloc(unsigned char *options, int optlen)
{
struct ppp_mppe_state *state;
+ struct crypto_shash *shash;
unsigned int digestsize;
if (optlen != CILEN_MPPE + sizeof(state->master_key) ||
@@ -217,13 +209,21 @@ static void *mppe_alloc(unsigned char *options, int optlen)
goto out_free;
}
- state->sha1 = crypto_alloc_ahash("sha1", 0, CRYPTO_ALG_ASYNC);
- if (IS_ERR(state->sha1)) {
- state->sha1 = NULL;
+ shash = crypto_alloc_shash("sha1", 0, 0);
+ if (IS_ERR(shash))
+ goto out_free;
+
+ state->sha1 = kmalloc(sizeof(*state->sha1) +
+ crypto_shash_descsize(shash),
+ GFP_KERNEL);
+ if (!state->sha1) {
+ crypto_free_shash(shash);
goto out_free;
}
+ state->sha1->tfm = shash;
+ state->sha1->flags = 0;
- digestsize = crypto_ahash_digestsize(state->sha1);
+ digestsize = crypto_shash_digestsize(shash);
if (digestsize < MPPE_MAX_KEY_LEN)
goto out_free;
@@ -246,7 +246,10 @@ static void *mppe_alloc(unsigned char *options, int optlen)
out_free:
kfree(state->sha1_digest);
- crypto_free_ahash(state->sha1);
+ if (state->sha1) {
+ crypto_free_shash(state->sha1->tfm);
+ kzfree(state->sha1);
+ }
crypto_free_skcipher(state->arc4);
kfree(state);
out:
@@ -261,7 +264,8 @@ static void mppe_free(void *arg)
struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
if (state) {
kfree(state->sha1_digest);
- crypto_free_ahash(state->sha1);
+ crypto_free_shash(state->sha1->tfm);
+ kzfree(state->sha1);
crypto_free_skcipher(state->arc4);
kfree(state);
}
--
2.17.1
^ permalink raw reply related
* [PATCH v6 15/18] staging: rtl8192e: ieee80211: Convert from ahash to shash
From: Kees Cook @ 2018-07-24 16:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Kees Cook, Arnd Bergmann, Eric Biggers, Gustavo A. R. Silva,
Alasdair Kergon, Rabin Vincent, Tim Chen, Rafael J. Wysocki,
Pavel Machek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Philipp Reisner, Lars Ellenberg, Jens Axboe, Giovanni Cabiddu,
Mike Snitzer, Paul Mackerras, Greg Kroah-Hartman,
David Howells <dhowe
In-Reply-To: <20180724164936.37477-1-keescook@chromium.org>
This is an identical change to the wireless/lib80211 of the same name.
In preparing to remove all stack VLA usage from the kernel[1], this
removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
to direct shash. By removing a layer of indirection this both improves
performance and reduces stack usage. The stack allocation will be made
a fixed size in a later patch to the crypto subsystem.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/staging/rtl8192e/rtllib_crypt_tkip.c | 56 ++++++++++----------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtllib_crypt_tkip.c b/drivers/staging/rtl8192e/rtllib_crypt_tkip.c
index ae103b0b7a2a..9f18be14dda6 100644
--- a/drivers/staging/rtl8192e/rtllib_crypt_tkip.c
+++ b/drivers/staging/rtl8192e/rtllib_crypt_tkip.c
@@ -50,9 +50,9 @@ struct rtllib_tkip_data {
int key_idx;
struct crypto_skcipher *rx_tfm_arc4;
- struct crypto_ahash *rx_tfm_michael;
+ struct crypto_shash *rx_tfm_michael;
struct crypto_skcipher *tx_tfm_arc4;
- struct crypto_ahash *tx_tfm_michael;
+ struct crypto_shash *tx_tfm_michael;
/* scratch buffers for virt_to_page() (crypto API) */
u8 rx_hdr[16];
u8 tx_hdr[16];
@@ -74,8 +74,7 @@ static void *rtllib_tkip_init(int key_idx)
goto fail;
}
- priv->tx_tfm_michael = crypto_alloc_ahash("michael_mic", 0,
- CRYPTO_ALG_ASYNC);
+ priv->tx_tfm_michael = crypto_alloc_shash("michael_mic", 0, 0);
if (IS_ERR(priv->tx_tfm_michael)) {
pr_debug("Could not allocate crypto API michael_mic\n");
priv->tx_tfm_michael = NULL;
@@ -90,8 +89,7 @@ static void *rtllib_tkip_init(int key_idx)
goto fail;
}
- priv->rx_tfm_michael = crypto_alloc_ahash("michael_mic", 0,
- CRYPTO_ALG_ASYNC);
+ priv->rx_tfm_michael = crypto_alloc_shash("michael_mic", 0, 0);
if (IS_ERR(priv->rx_tfm_michael)) {
pr_debug("Could not allocate crypto API michael_mic\n");
priv->rx_tfm_michael = NULL;
@@ -101,9 +99,9 @@ static void *rtllib_tkip_init(int key_idx)
fail:
if (priv) {
- crypto_free_ahash(priv->tx_tfm_michael);
+ crypto_free_shash(priv->tx_tfm_michael);
crypto_free_skcipher(priv->tx_tfm_arc4);
- crypto_free_ahash(priv->rx_tfm_michael);
+ crypto_free_shash(priv->rx_tfm_michael);
crypto_free_skcipher(priv->rx_tfm_arc4);
kfree(priv);
}
@@ -117,9 +115,9 @@ static void rtllib_tkip_deinit(void *priv)
struct rtllib_tkip_data *_priv = priv;
if (_priv) {
- crypto_free_ahash(_priv->tx_tfm_michael);
+ crypto_free_shash(_priv->tx_tfm_michael);
crypto_free_skcipher(_priv->tx_tfm_arc4);
- crypto_free_ahash(_priv->rx_tfm_michael);
+ crypto_free_shash(_priv->rx_tfm_michael);
crypto_free_skcipher(_priv->rx_tfm_arc4);
}
kfree(priv);
@@ -504,29 +502,31 @@ static int rtllib_tkip_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
}
-static int michael_mic(struct crypto_ahash *tfm_michael, u8 *key, u8 *hdr,
+static int michael_mic(struct crypto_shash *tfm_michael, u8 *key, u8 *hdr,
u8 *data, size_t data_len, u8 *mic)
{
- AHASH_REQUEST_ON_STACK(req, tfm_michael);
- struct scatterlist sg[2];
+ SHASH_DESC_ON_STACK(desc, tfm_michael);
int err;
- if (tfm_michael == NULL) {
- pr_warn("michael_mic: tfm_michael == NULL\n");
- return -1;
- }
- sg_init_table(sg, 2);
- sg_set_buf(&sg[0], hdr, 16);
- sg_set_buf(&sg[1], data, data_len);
+ desc->tfm = tfm_michael;
+ desc->flags = 0;
- if (crypto_ahash_setkey(tfm_michael, key, 8))
+ if (crypto_shash_setkey(tfm_michael, key, 8))
return -1;
- ahash_request_set_tfm(req, tfm_michael);
- ahash_request_set_callback(req, 0, NULL, NULL);
- ahash_request_set_crypt(req, sg, mic, data_len + 16);
- err = crypto_ahash_digest(req);
- ahash_request_zero(req);
+ err = crypto_shash_init(desc);
+ if (err)
+ goto out;
+ err = crypto_shash_update(desc, hdr, 16);
+ if (err)
+ goto out;
+ err = crypto_shash_update(desc, data, data_len);
+ if (err)
+ goto out;
+ err = crypto_shash_final(desc, mic);
+
+out:
+ shash_desc_zero(desc);
return err;
}
@@ -663,9 +663,9 @@ static int rtllib_tkip_set_key(void *key, int len, u8 *seq, void *priv)
{
struct rtllib_tkip_data *tkey = priv;
int keyidx;
- struct crypto_ahash *tfm = tkey->tx_tfm_michael;
+ struct crypto_shash *tfm = tkey->tx_tfm_michael;
struct crypto_skcipher *tfm2 = tkey->tx_tfm_arc4;
- struct crypto_ahash *tfm3 = tkey->rx_tfm_michael;
+ struct crypto_shash *tfm3 = tkey->rx_tfm_michael;
struct crypto_skcipher *tfm4 = tkey->rx_tfm_arc4;
keyidx = tkey->key_idx;
--
2.17.1
^ permalink raw reply related
* [PATCH v6 16/18] rxrpc: Reuse SKCIPHER_REQUEST_ON_STACK buffer
From: Kees Cook @ 2018-07-24 16:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Kees Cook, Arnd Bergmann, Eric Biggers, Gustavo A. R. Silva,
Alasdair Kergon, Rabin Vincent, Tim Chen, Rafael J. Wysocki,
Pavel Machek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Philipp Reisner, Lars Ellenberg, Jens Axboe, Giovanni Cabiddu,
Mike Snitzer, Paul Mackerras, Greg Kroah-Hartman,
David Howells <dhowe
In-Reply-To: <20180724164936.37477-1-keescook@chromium.org>
The use of SKCIPHER_REQUEST_ON_STACK() will trigger FRAME_WARN warnings
(when less than 2048) once the VLA is no longer hidden from the check:
net/rxrpc/rxkad.c:398:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
net/rxrpc/rxkad.c:242:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
This passes the initial SKCIPHER_REQUEST_ON_STACK allocation to the leaf
functions for reuse. Two requests allocated on the stack is not needed
when only one is used at a time.
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
net/rxrpc/rxkad.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 278ac0807a60..6393391fac86 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -146,10 +146,10 @@ static int rxkad_prime_packet_security(struct rxrpc_connection *conn)
static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
struct sk_buff *skb,
u32 data_size,
- void *sechdr)
+ void *sechdr,
+ struct skcipher_request *req)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxkad_level1_hdr hdr;
struct rxrpc_crypt iv;
struct scatterlist sg;
@@ -183,12 +183,12 @@ static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
struct sk_buff *skb,
u32 data_size,
- void *sechdr)
+ void *sechdr,
+ struct skcipher_request *req)
{
const struct rxrpc_key_token *token;
struct rxkad_level2_hdr rxkhdr;
struct rxrpc_skb_priv *sp;
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxrpc_crypt iv;
struct scatterlist sg[16];
struct sk_buff *trailer;
@@ -296,11 +296,12 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
ret = 0;
break;
case RXRPC_SECURITY_AUTH:
- ret = rxkad_secure_packet_auth(call, skb, data_size, sechdr);
+ ret = rxkad_secure_packet_auth(call, skb, data_size, sechdr,
+ req);
break;
case RXRPC_SECURITY_ENCRYPT:
ret = rxkad_secure_packet_encrypt(call, skb, data_size,
- sechdr);
+ sechdr, req);
break;
default:
ret = -EPERM;
@@ -316,10 +317,10 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
*/
static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
unsigned int offset, unsigned int len,
- rxrpc_seq_t seq)
+ rxrpc_seq_t seq,
+ struct skcipher_request *req)
{
struct rxkad_level1_hdr sechdr;
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxrpc_crypt iv;
struct scatterlist sg[16];
struct sk_buff *trailer;
@@ -402,11 +403,11 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
*/
static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
unsigned int offset, unsigned int len,
- rxrpc_seq_t seq)
+ rxrpc_seq_t seq,
+ struct skcipher_request *req)
{
const struct rxrpc_key_token *token;
struct rxkad_level2_hdr sechdr;
- SKCIPHER_REQUEST_ON_STACK(req, call->conn->cipher);
struct rxrpc_crypt iv;
struct scatterlist _sg[4], *sg;
struct sk_buff *trailer;
@@ -549,9 +550,9 @@ static int rxkad_verify_packet(struct rxrpc_call *call, struct sk_buff *skb,
case RXRPC_SECURITY_PLAIN:
return 0;
case RXRPC_SECURITY_AUTH:
- return rxkad_verify_packet_1(call, skb, offset, len, seq);
+ return rxkad_verify_packet_1(call, skb, offset, len, seq, req);
case RXRPC_SECURITY_ENCRYPT:
- return rxkad_verify_packet_2(call, skb, offset, len, seq);
+ return rxkad_verify_packet_2(call, skb, offset, len, seq, req);
default:
return -ENOANO;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 0/9] Netfilter fixes for net
From: David Miller @ 2018-07-24 17:00 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <20180724163133.14586-1-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 24 Jul 2018 18:31:24 +0200
> The following patchset contains Netfilter fixes for net:
...
> You can pull these changes from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git
Pulled, thank you.
^ permalink raw reply
* Re: pull-request: mac80211 2018-07-24
From: David Miller @ 2018-07-24 17:00 UTC (permalink / raw)
To: johannes; +Cc: netdev, linux-wireless
In-Reply-To: <20180724071658.9464-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 24 Jul 2018 09:16:57 +0200
> After a long delay (I'm still on vacation & bonding leave until
> mid August and forgot), here are a few fixes for the current
> release cycle.
>
> Please pull and let me know if there's any problem.
Pulled, thanks Johannes.
^ permalink raw reply
* Re: pull-request: mac80211-next 2018-07-24
From: David Miller @ 2018-07-24 17:03 UTC (permalink / raw)
To: johannes; +Cc: netdev, linux-wireless
In-Reply-To: <20180724072558.15099-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 24 Jul 2018 09:25:57 +0200
> Also have a few changes for -next, figured I should get this
> out too.
>
> Please pull and let me know if there's any problem.
Also pulled, thanks a lot!
^ permalink raw reply
* Re: [PATCH net-next] netlink: do not store start function in netlink_cb
From: David Miller @ 2018-07-24 17:05 UTC (permalink / raw)
To: fw; +Cc: netdev
In-Reply-To: <20180724104756.29702-1-fw@strlen.de>
From: Florian Westphal <fw@strlen.de>
Date: Tue, 24 Jul 2018 12:47:56 +0200
> ->start() is called once when dump is being initialized, there is no
> need to store it in netlink_cb.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
Nice simplification, applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives
From: Yuchung Cheng @ 2018-07-24 17:06 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Neal Cardwell, Lawrence Brakmo, Netdev, Kernel Team,
Alexei Starovoitov, Eric Dumazet
In-Reply-To: <3c785042-0b9a-69cd-9415-e8a10ea0baf7@iogearbox.net>
On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 07/24/2018 04:15 AM, Neal Cardwell wrote:
> > On Mon, Jul 23, 2018 at 8:49 PM Lawrence Brakmo <brakmo@fb.com> wrote:
> >>
> >> We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
> >> problem is triggered when the last packet of a request arrives CE
> >> marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
> >> to 1 (because there are no packets in flight). When the 1st packet of
> >> the next request arrives, the ACK was sometimes delayed even though it
> >> is CWR marked, adding up to 40ms to the RPC latency.
> >>
> >> This patch insures that CWR marked data packets arriving will be acked
> >> immediately.
> > ...
> >> Modified based on comments by Neal Cardwell <ncardwell@google.com>
> >>
> >> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> >> ---
> >> net/ipv4/tcp_input.c | 9 ++++++++-
> >> 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > Seems like a nice mechanism to have, IMHO.
> >
> > Acked-by: Neal Cardwell <ncardwell@google.com>
>
> Should this go to net tree instead where all the other fixes went?
I am neutral but this feels more like a feature improvement
Acked-by: Yuchung Cheng <ycheng@google.com>
btw this should also help the classic ECN case upon timeout that
triggers one packet retransmission.
>
> Thanks,
> Daniel
^ permalink raw reply
* Re: [PATCH net-next 0/4] mlxsw: Add extack messages for tc flower
From: David Miller @ 2018-07-24 17:10 UTC (permalink / raw)
To: idosch; +Cc: netdev, nird, jiri, mlxsw
In-Reply-To: <20180724141314.10728-1-idosch@mellanox.com>
From: Ido Schimmel <idosch@mellanox.com>
Date: Tue, 24 Jul 2018 17:13:10 +0300
> Nir says:
>
> This patch set adds extack messages support to tc flower part of mlxsw.
> The messages provide clear reasoning to failures, as some of the available
> actions and keys are not supported in driver or HW and resources may get
> exhausted.
>
> The first patch deals with propagation of the extack pointer among the functions
> dealing with key parsing and action sets handling.
>
> Following patches 2-4 add appropriate messages across the different layers of
> mlxsw tc flower implementation.
Looks good, series applied.
^ permalink raw reply
* Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives
From: Neal Cardwell @ 2018-07-24 17:12 UTC (permalink / raw)
To: Yuchung Cheng
Cc: Daniel Borkmann, Lawrence Brakmo, Netdev, Kernel Team, ast,
Eric Dumazet
In-Reply-To: <CAK6E8=dc_k+EBON3XJZsA3nedsVD8ztHovpkp0mk_5M13nqWNw@mail.gmail.com>
On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng <ycheng@google.com> wrote:
>
> On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Should this go to net tree instead where all the other fixes went?
> I am neutral but this feels more like a feature improvement
I agree this feels like a feature improvement rather than a bug fix.
neal
^ permalink raw reply
* Re: [PATCH net-next 0/2] cxgb4: collect free Tx/Rx pages and page pointers
From: David Miller @ 2018-07-24 17:12 UTC (permalink / raw)
To: rahul.lakkireddy; +Cc: netdev, ganeshgr, nirranjan, indranil
In-Reply-To: <cover.1532166615.git.rahul.lakkireddy@chelsio.com>
From: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Date: Tue, 24 Jul 2018 20:17:08 +0530
> Patch 1 collects number of free PSTRUCT page pointers in context
> memory.
>
> Patch 2 moves the collection logic for Tx/Rx free pages to common
> code, since this information needs to be collected in vmcore device
> dump as well.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: add End.DT6 action to bpf_lwt_seg6_action helper
From: Martin KaFai Lau @ 2018-07-24 17:14 UTC (permalink / raw)
To: Mathieu Xhonneux; +Cc: netdev, daniel, alexei.starovoitov
In-Reply-To: <20180724165954.5823-1-m.xhonneux@gmail.com>
On Tue, Jul 24, 2018 at 04:59:54PM +0000, Mathieu Xhonneux wrote:
> The seg6local LWT provides the End.DT6 action, which allows to
> decapsulate an outer IPv6 header containing a Segment Routing Header
> (SRH), full specification is available here:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dfilsfils-2Dspring-2Dsrv6-2Dnetwork-2Dprogramming-2D05&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=c61PGnhPMmCUcL5lpyBsxOmsBU2mU5KFY0-Ioo-pBC4&s=mzShtRc5ofzfknAuqoehbGN1ifA17aKihiVLJVfkuZ8&e=
>
> This patch adds this action now to the seg6local BPF
> interface. Since it is not mandatory that the inner IPv6 header also
> contains a SRH, seg6_bpf_srh_state has been extended with a pointer to
> a possible SRH of the outermost IPv6 header. This helps assessing if the
> validation must be triggered or not, and avoids some calls to
> ipv6_find_hdr.
>
> Signed-off-by: Mathieu Xhonneux <m.xhonneux@gmail.com>
> ---
> include/net/seg6_local.h | 4 ++-
> net/core/filter.c | 83 +++++++++++++++++++++++++++++++++---------------
> net/ipv6/seg6_local.c | 42 +++++++++++++++---------
> 3 files changed, 87 insertions(+), 42 deletions(-)
>
> diff --git a/include/net/seg6_local.h b/include/net/seg6_local.h
> index 661fd5b4d3e0..08359e2d8b35 100644
> --- a/include/net/seg6_local.h
> +++ b/include/net/seg6_local.h
> @@ -21,10 +21,12 @@
>
> extern int seg6_lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
> u32 tbl_id);
> +extern bool seg6_bpf_has_valid_srh(struct sk_buff *skb);
>
> struct seg6_bpf_srh_state {
> - bool valid;
> + struct ipv6_sr_hdr *srh;
> u16 hdrlen;
> + bool valid;
> };
>
> DECLARE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states);
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 104d560946da..2cdea7d05063 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4542,14 +4542,13 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
> {
> struct seg6_bpf_srh_state *srh_state =
> this_cpu_ptr(&seg6_bpf_srh_states);
> + struct ipv6_sr_hdr *srh = srh_state->srh;
> void *srh_tlvs, *srh_end, *ptr;
> - struct ipv6_sr_hdr *srh;
> int srhoff = 0;
>
> - if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> + if (srh == NULL)
> return -EINVAL;
>
> - srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> srh_tlvs = (void *)((char *)srh + ((srh->first_segment + 1) << 4));
> srh_end = (void *)((char *)srh + sizeof(*srh) + srh_state->hdrlen);
>
> @@ -4562,6 +4561,9 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
>
> if (unlikely(bpf_try_make_writable(skb, offset + len)))
> return -EFAULT;
> + if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> + return -EINVAL;
> + srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>
> memcpy(skb->data + offset, from, len);
> return 0;
> @@ -4577,52 +4579,79 @@ static const struct bpf_func_proto bpf_lwt_seg6_store_bytes_proto = {
> .arg4_type = ARG_CONST_SIZE
> };
>
> -BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
> - u32, action, void *, param, u32, param_len)
> +static void bpf_update_srh_state(struct sk_buff *skb)
> {
> struct seg6_bpf_srh_state *srh_state =
> this_cpu_ptr(&seg6_bpf_srh_states);
> - struct ipv6_sr_hdr *srh;
> int srhoff = 0;
> - int err;
> -
> - if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> - return -EINVAL;
> - srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> -
> - if (!srh_state->valid) {
> - if (unlikely((srh_state->hdrlen & 7) != 0))
> - return -EBADMSG;
> -
> - srh->hdrlen = (u8)(srh_state->hdrlen >> 3);
> - if (unlikely(!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3)))
> - return -EBADMSG;
>
> + if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0) {
> + srh_state->srh = NULL;
> + } else {
> + srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> + srh_state->hdrlen = srh_state->srh->hdrlen << 3;
> srh_state->valid = 1;
> }
> +}
> +
> +BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
> + u32, action, void *, param, u32, param_len)
> +{
> + struct seg6_bpf_srh_state *srh_state =
> + this_cpu_ptr(&seg6_bpf_srh_states);
> + int hdroff = 0;
> + int err;
>
> switch (action) {
> case SEG6_LOCAL_ACTION_END_X:
> + if (!seg6_bpf_has_valid_srh(skb))
> + return -EBADMSG;
> if (param_len != sizeof(struct in6_addr))
> return -EINVAL;
> return seg6_lookup_nexthop(skb, (struct in6_addr *)param, 0);
> case SEG6_LOCAL_ACTION_END_T:
> + if (!seg6_bpf_has_valid_srh(skb))
> + return -EBADMSG;
> + if (param_len != sizeof(int))
> + return -EINVAL;
> + return seg6_lookup_nexthop(skb, NULL, *(int *)param);
> + case SEG6_LOCAL_ACTION_END_DT6:
> + if (!seg6_bpf_has_valid_srh(skb))
> + return -EBADMSG;
> if (param_len != sizeof(int))
> return -EINVAL;
> +
> + // find inner IPv6 header, pull outer IPv6 header
> + if (ipv6_find_hdr(skb, &hdroff, IPPROTO_IPV6, NULL, NULL) < 0)
> + return -EBADMSG;
> + if (!pskb_pull(skb, hdroff))
> + return -EBADMSG;
> +
> + skb_postpull_rcsum(skb, skb_network_header(skb), hdroff);
> + skb_reset_network_header(skb);
> + skb_reset_transport_header(skb);
> + skb->encapsulation = 0;
> +
> + bpf_compute_data_pointers(skb);
> + bpf_update_srh_state(skb);
> return seg6_lookup_nexthop(skb, NULL, *(int *)param);
> case SEG6_LOCAL_ACTION_END_B6:
> + if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
> + return -EBADMSG;
> err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6_INLINE,
> param, param_len);
> if (!err)
> - srh_state->hdrlen =
> - ((struct ipv6_sr_hdr *)param)->hdrlen << 3;
> + bpf_update_srh_state(skb);
> +
> return err;
> case SEG6_LOCAL_ACTION_END_B6_ENCAP:
> + if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
> + return -EBADMSG;
> err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6,
> param, param_len);
> if (!err)
> - srh_state->hdrlen =
> - ((struct ipv6_sr_hdr *)param)->hdrlen << 3;
> + bpf_update_srh_state(skb);
> +
> return err;
> default:
> return -EINVAL;
> @@ -4644,15 +4673,14 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
> {
> struct seg6_bpf_srh_state *srh_state =
> this_cpu_ptr(&seg6_bpf_srh_states);
> + struct ipv6_sr_hdr *srh = srh_state->srh;
> void *srh_end, *srh_tlvs, *ptr;
> - struct ipv6_sr_hdr *srh;
> struct ipv6hdr *hdr;
> int srhoff = 0;
> int ret;
>
> - if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> + if (unlikely(srh == NULL))
> return -EINVAL;
> - srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>
> srh_tlvs = (void *)((unsigned char *)srh + sizeof(*srh) +
> ((srh->first_segment + 1) << 4));
> @@ -4682,6 +4710,9 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
> hdr = (struct ipv6hdr *)skb->data;
> hdr->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
>
> + if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> + return -EINVAL;
> + srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> srh_state->hdrlen += len;
> srh_state->valid = 0;
> return 0;
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index e1025b493a18..461f99d5043c 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -459,14 +459,35 @@ static int input_action_end_b6_encap(struct sk_buff *skb,
>
> DEFINE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states);
>
> +bool seg6_bpf_has_valid_srh(struct sk_buff *skb)
> +{
> + struct seg6_bpf_srh_state *srh_state =
> + this_cpu_ptr(&seg6_bpf_srh_states);
> + struct ipv6_sr_hdr *srh = srh_state->srh;
> +
> + if (unlikely(srh == NULL))
> + return false;
> +
> + if (unlikely(!srh_state->valid)) {
> + if ((srh_state->hdrlen & 7) != 0)
> + return false;
> +
> + srh->hdrlen = (u8)(srh_state->hdrlen >> 3);
> + if (!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3))
> + return false;
> +
> + srh_state->valid = 1;
Nit. s/1/true/
> + }
> +
> + return true;
> +}
> +
> static int input_action_end_bpf(struct sk_buff *skb,
> struct seg6_local_lwt *slwt)
> {
> struct seg6_bpf_srh_state *srh_state =
> this_cpu_ptr(&seg6_bpf_srh_states);
> - struct seg6_bpf_srh_state local_srh_state;
> struct ipv6_sr_hdr *srh;
> - int srhoff = 0;
> int ret;
>
> srh = get_and_validate_srh(skb);
> @@ -478,6 +499,7 @@ static int input_action_end_bpf(struct sk_buff *skb,
> * which is also accessed by the bpf_lwt_seg6_* helpers
> */
> preempt_disable();
> + srh_state->srh = srh;
> srh_state->hdrlen = srh->hdrlen << 3;
> srh_state->valid = 1;
>
> @@ -486,9 +508,6 @@ static int input_action_end_bpf(struct sk_buff *skb,
> ret = bpf_prog_run_save_cb(slwt->bpf.prog, skb);
> rcu_read_unlock();
>
> - local_srh_state = *srh_state;
> - preempt_enable();
> -
> switch (ret) {
> case BPF_OK:
> case BPF_REDIRECT:
> @@ -500,24 +519,17 @@ static int input_action_end_bpf(struct sk_buff *skb,
> goto drop;
> }
>
> - if (unlikely((local_srh_state.hdrlen & 7) != 0))
> - goto drop;
> -
> - if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> - goto drop;
> - srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> - srh->hdrlen = (u8)(local_srh_state.hdrlen >> 3);
> -
> - if (!local_srh_state.valid &&
> - unlikely(!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3)))
> + if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
> goto drop;
>
> + preempt_enable();
> if (ret != BPF_REDIRECT)
> seg6_lookup_nexthop(skb, NULL, 0);
>
> return dst_input(skb);
>
> drop:
> + preempt_enable();
For this drop case at the beginning of this function:
srh = get_and_validate_srh(skb);
if (!srh)
goto drop;
preempt_disable() was not called yet?
> kfree_skb(skb);
> return -EINVAL;
> }
> --
> 2.16.1
>
^ permalink raw reply
* Re: [PATCH net] net/ipv6: Fix linklocal to global address with VRF
From: David Ahern @ 2018-07-24 17:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20180721.193236.2034280150310979114.davem@davemloft.net>
On 7/21/18 8:32 PM, David Miller wrote:
>
> Applied and queued up for -stable.
>
>> Dave: I can look at the backports to stable if needed.
>
> Please do, that will help me a lot.
>
It applies cleanly to 4.17 and 4.14. I tested 4.14 it is fine. Since
4.18 and 4.14 work I have no reason to believe 4.17 will have a problem.
Only other relevant release is 4.9 which is handled by others. It needs
some non-trivial work to backport so needs to wait until after PTO.
^ permalink raw reply
* Re: [PATCHv3 net-next 2/2] selftests: add a selftest for directed broadcast forwarding
From: Xin Long @ 2018-07-24 17:24 UTC (permalink / raw)
To: David Ahern; +Cc: network dev, davem, Davide Caratti, Ido Schimmel
In-Reply-To: <5af03df3-cad5-161e-31a8-3921fd64b98a@gmail.com>
On Mon, Jul 23, 2018 at 11:17 PM, David Ahern <dsahern@gmail.com> wrote:
> On 7/23/18 5:51 AM, Xin Long wrote:
>> +ping_ipv4()
>> +{
>> + sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
>> +
>> + bc_forwarding_disable
>> + ping_test_from $h1 198.51.100.255 192.0.2.1
>> + ping_test_from $h1 198.51.200.255 192.0.2.1
>> + ping_test_from $h1 192.0.2.255 192.0.2.1
>> + ping_test_from $h1 255.255.255.255 192.0.2.1
>> +
>> + ping_test_from $h2 192.0.2.255 198.51.100.1
>> + ping_test_from $h2 198.51.200.255 198.51.100.1
>> + ping_test_from $h2 198.51.100.255 198.51.100.1
>> + ping_test_from $h2 255.255.255.255 198.51.100.1
>> + bc_forwarding_restore
>> +
>> + bc_forwarding_enable
>> + ping_test_from $h1 198.51.100.255 198.51.100.2
>> + ping_test_from $h1 198.51.200.255 198.51.200.2
>> + ping_test_from $h1 192.0.2.255 192.0.2.1 1
>> + ping_test_from $h1 255.255.255.255 192.0.2.1
>> +
>> + ping_test_from $h2 192.0.2.255 192.0.2.2
>> + ping_test_from $h2 198.51.200.255 198.51.200.2
>> + ping_test_from $h2 198.51.100.255 198.51.100.1 1
>> + ping_test_from $h2 255.255.255.255 198.51.100.1
>> + bc_forwarding_restore
>> +
>> + sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
>
> You need a better description for each test. This output:
> TEST: ping_test_from [PASS]
> TEST: ping_test_from [PASS]
> TEST: ping_test_from [PASS]
> TEST: ping_test_from [PASS]
> ...
>
> does not help in understanding which cases are working and which are not.
# ./router_broadcast.sh
INFO: bc_forwarding disabled on r1=>
INFO: h1 -> net2: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
INFO: h1 -> net3: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
INFO: h1 -> net1: reply from r1 (not dropping)
TEST: ping_test_from [PASS]
INFO: h1 -> 255.255.255.255: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
INFO: h2 -> net1: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
INFO: h2 -> net3: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
INFO: h2 -> net2: reply from r1 (not dropping)
TEST: ping_test_from [PASS]
INFO: h2 -> 255.255.255.255: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
INFO: bc_forwarding enabled on r1 =>
INFO: h1 -> net2: reply from h2 (forwarding)
TEST: ping_test_from [PASS]
INFO: h1 -> net3: reply from h3 (forwarding)
TEST: ping_test_from [PASS]
INFO: h1 -> net1: no reply (dropping)
TEST: ping_test_from [PASS]
INFO: h1 -> 255.255.255.255: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
INFO: h2 -> net1: reply from h3 (forwarding)
TEST: ping_test_from [PASS]
INFO: h2 -> net3: reply from h1 (forwarding)
TEST: ping_test_from [PASS]
INFO: h2 -> net2: no reply (dropping)
TEST: ping_test_from [PASS]
INFO: h2 -> 255.255.255.255: reply from r1 (not forwarding)
TEST: ping_test_from [PASS]
I hope this log looks good to you?
^ permalink raw reply
* Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives
From: Eric Dumazet @ 2018-07-24 17:26 UTC (permalink / raw)
To: Neal Cardwell, Yuchung Cheng
Cc: Daniel Borkmann, Lawrence Brakmo, Netdev, Kernel Team, ast,
Eric Dumazet
In-Reply-To: <CADVnQynbJPtHSJuYoS6fEKdo-2Q_P4VXAa=ytz+NHMM+0dcLmA@mail.gmail.com>
On 07/24/2018 10:12 AM, Neal Cardwell wrote:
> On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng <ycheng@google.com> wrote:
>>
>> On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> Should this go to net tree instead where all the other fixes went?
>> I am neutral but this feels more like a feature improvement
>
> I agree this feels like a feature improvement rather than a bug fix.
Agreed
Signed-off-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [iproute PATCH] lib/namespace: avoid double-mounting a /sys
From: Lubomir Rintel @ 2018-07-24 17:26 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger, Phil Sutter, Lubomir Rintel
This partly reverts 8f0807023d067e2bb585a2ae8da93e59689d10f1, bringing
back the umount(/sys) attempt.
In a LXC container we're unable to umount the sysfs instance, nor mount
a read-write one. We still are able to create a new read-only instance.
Nevertheless, it still makes sense to attempt the umount() even though
the sysfs is mounted read-only. Otherwise we may end up attempting to
mount a sysfs with the same flags as is already mounted, resulting in
an EBUSY error (meaning "Already mounted").
Perhaps this is not a very likely scenario in real world, but we hit
it in NetworkManager test suite and makes netns_switch() somewhat more
robust. It also fixes the case, when /sys wasn't mounted at all.
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
lib/namespace.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/lib/namespace.c b/lib/namespace.c
index 43e0fe34..06ae0a48 100644
--- a/lib/namespace.c
+++ b/lib/namespace.c
@@ -82,19 +82,13 @@ int netns_switch(char *name)
/* Mount a version of /sys that describes the network namespace */
- if (statvfs("/sys", &fsstat) < 0) {
- fprintf(stderr, "could not stat /sys (not mounted?): %s\n",strerror(errno));
- return -1;
- }
- if (fsstat.f_flag & ST_RDONLY) {
- /* If /sys is not writable (e.g. in a container), we can't
- * unmount the old /sys instance, but we can still mount a new
- * read-only instance over it. */
- mountflags = MS_RDONLY;
- } else {
- if (umount2("/sys", MNT_DETACH) < 0) {
- fprintf(stderr, "umount of /sys failed: %s\n", strerror(errno));
- return -1;
+ if (umount2("/sys", MNT_DETACH) < 0) {
+ /* If this fails, perhaps there wasn't a sysfs instance mounted. Good. */
+ if (statvfs("/sys", &fsstat) == 0) {
+ /* We couldn't umount the sysfs, we'll attempt to overlay it.
+ * A read-only instance can't be shadowed with a read-write one. */
+ if (fsstat.f_flag & ST_RDONLY)
+ mountflags = MS_RDONLY;
}
}
if (mount(name, "/sys", "sysfs", mountflags, NULL) < 0) {
--
2.17.1
^ permalink raw reply related
* Re: bisected: 4.18-rc* regression: x86-32 troubles (with timers?)
From: Daniel Borkmann @ 2018-07-24 18:40 UTC (permalink / raw)
To: Meelis Roos; +Cc: Arnd Bergmann, Linux Kernel list, Networking
In-Reply-To: <alpine.LRH.2.21.1807240744570.11834@math.ut.ee>
On 07/24/2018 06:47 AM, Meelis Roos wrote:
>>> Anyway, I started compile of v4.18-rc5 that was the latest I tested,
>>> with the commit in question reverted. Will see if I can test tomorrow
>>> morning. But I will leave tomorrow for a week and can only test further
>>> things if they happen to boot fine (no manual reboot possible for a
>>> week).
>>
>> Ok, thanks, please keep us posted on the outcome with the revert. Right
>> now I would doubt it's related resp. changes anything on the issue, but
>> lets see.
>
> v4.18-rc5 minus the patch in question worked fine on 2 bootups so it
> seems to be good.
And this was with CONFIG_BPF_JIT_ALWAYS_ON=y, correct? Could you try the
same with this below applied:
diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
index 5579987..313c983 100644
--- a/arch/x86/net/bpf_jit_comp32.c
+++ b/arch/x86/net/bpf_jit_comp32.c
@@ -175,7 +175,7 @@ static const u8 bpf2ia32[][2] = {
#define SCRATCH_SIZE 96
/* Total stack size used in JITed code */
-#define _STACK_SIZE (stack_depth + SCRATCH_SIZE)
+#define _STACK_SIZE (stack_depth + SCRATCH_SIZE + 4)
#define STACK_SIZE ALIGN(_STACK_SIZE, STACK_ALIGNMENT)
Thanks,
Daniel
^ permalink raw reply related
* Re: [PATCH net-next 0/2] net/sctp: Avoid allocating high order memory with kmalloc()
From: Marcelo Ricardo Leitner @ 2018-07-24 17:36 UTC (permalink / raw)
To: Konstantin Khorenko
Cc: oleg.babin, netdev, linux-sctp, David S. Miller, Vlad Yasevich,
Neil Horman, Xin Long, Andrey Ryabinin
In-Reply-To: <b5ab1199-629b-1803-7826-ad74200bb34d@virtuozzo.com>
On Tue, Jul 24, 2018 at 06:35:35PM +0300, Konstantin Khorenko wrote:
> Hi Marcelo,
>
> pity to abandon Oleg's attempt to avoid high order allocations and use
> flex_array instead, so i tried to do the performance measurements with
> options you kindly suggested.
Nice, thanks!
...
> As we can see single stream tests do not show any noticeable degradation,
> and SCTP_*_MANY tests spread decreased significantly when -S/-s options are used,
> but still too big to consider the performance test pass or fail.
>
> Can you please advise anything else to try - to decrease the dispersion rate -
In addition, you can try also using a veth tunnel or reducing lo mtu
down to 1500, and also make use of sctp tests (need to be after the --
) option -m 1452. These will alleaviate issues with cwnd handling
that happen on loopback due to the big MTU and minimize issues with
rwnd/buffer size too.
Even with -S, -s, -m and the lower MTU, it is usual to see some
fluctuation, but not that much.
> or can we just consider values are fine and i'm reworking the patch according
> to your comment about sctp_stream_in(asoc, sid)/sctp_stream_in_ptr(stream, sid)
> and that's it?
Ok, thanks. It seems so, yes.
Marcelo
^ permalink raw reply
* Re: [PATCHv3 net-next 2/2] selftests: add a selftest for directed broadcast forwarding
From: David Ahern @ 2018-07-24 17:37 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, davem, Davide Caratti, Ido Schimmel
In-Reply-To: <CADvbK_dhnyZwqzff64ZQBggL8LcY__ot_h3Cj7Y-SOx1FxGbCw@mail.gmail.com>
On 7/24/18 11:24 AM, Xin Long wrote:
> On Mon, Jul 23, 2018 at 11:17 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 7/23/18 5:51 AM, Xin Long wrote:
>>> +ping_ipv4()
>>> +{
>>> + sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
>>> +
>>> + bc_forwarding_disable
>>> + ping_test_from $h1 198.51.100.255 192.0.2.1
>>> + ping_test_from $h1 198.51.200.255 192.0.2.1
>>> + ping_test_from $h1 192.0.2.255 192.0.2.1
>>> + ping_test_from $h1 255.255.255.255 192.0.2.1
>>> +
>>> + ping_test_from $h2 192.0.2.255 198.51.100.1
>>> + ping_test_from $h2 198.51.200.255 198.51.100.1
>>> + ping_test_from $h2 198.51.100.255 198.51.100.1
>>> + ping_test_from $h2 255.255.255.255 198.51.100.1
>>> + bc_forwarding_restore
>>> +
>>> + bc_forwarding_enable
>>> + ping_test_from $h1 198.51.100.255 198.51.100.2
>>> + ping_test_from $h1 198.51.200.255 198.51.200.2
>>> + ping_test_from $h1 192.0.2.255 192.0.2.1 1
>>> + ping_test_from $h1 255.255.255.255 192.0.2.1
>>> +
>>> + ping_test_from $h2 192.0.2.255 192.0.2.2
>>> + ping_test_from $h2 198.51.200.255 198.51.200.2
>>> + ping_test_from $h2 198.51.100.255 198.51.100.1 1
>>> + ping_test_from $h2 255.255.255.255 198.51.100.1
>>> + bc_forwarding_restore
>>> +
>>> + sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
>>
>> You need a better description for each test. This output:
>> TEST: ping_test_from [PASS]
>> TEST: ping_test_from [PASS]
>> TEST: ping_test_from [PASS]
>> TEST: ping_test_from [PASS]
>> ...
>>
>> does not help in understanding which cases are working and which are not.
> # ./router_broadcast.sh
> INFO: bc_forwarding disabled on r1=>
> INFO: h1 -> net2: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
> INFO: h1 -> net3: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
> INFO: h1 -> net1: reply from r1 (not dropping)
> TEST: ping_test_from [PASS]
> INFO: h1 -> 255.255.255.255: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
> INFO: h2 -> net1: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
> INFO: h2 -> net3: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
> INFO: h2 -> net2: reply from r1 (not dropping)
> TEST: ping_test_from [PASS]
> INFO: h2 -> 255.255.255.255: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
> INFO: bc_forwarding enabled on r1 =>
> INFO: h1 -> net2: reply from h2 (forwarding)
> TEST: ping_test_from [PASS]
> INFO: h1 -> net3: reply from h3 (forwarding)
> TEST: ping_test_from [PASS]
> INFO: h1 -> net1: no reply (dropping)
> TEST: ping_test_from [PASS]
> INFO: h1 -> 255.255.255.255: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
> INFO: h2 -> net1: reply from h3 (forwarding)
> TEST: ping_test_from [PASS]
> INFO: h2 -> net3: reply from h1 (forwarding)
> TEST: ping_test_from [PASS]
> INFO: h2 -> net2: no reply (dropping)
> TEST: ping_test_from [PASS]
> INFO: h2 -> 255.255.255.255: reply from r1 (not forwarding)
> TEST: ping_test_from [PASS]
>
> I hope this log looks good to you?
>
The extra INFO is good, but the TEST line needs a better description.
^ permalink raw reply
* Re: [PATCH net-next] tcp: ack immediately when a cwr packet arrives
From: Lawrence Brakmo @ 2018-07-24 17:42 UTC (permalink / raw)
To: Neal Cardwell, Yuchung Cheng
Cc: Daniel Borkmann, Netdev, Kernel Team, Alexei Starovoitov,
Eric Dumazet
In-Reply-To: <CADVnQynbJPtHSJuYoS6fEKdo-2Q_P4VXAa=ytz+NHMM+0dcLmA@mail.gmail.com>
Note that without this fix the 99% latencies when doing 10KB RPCs in a congested network using DCTCP are 40ms vs. 190us with the patch. Also note that these 40ms high tail latencies started after commit 3759824da87b30ce7a35b4873b62b0ba38905ef5 in Jul 2015, which triggered the bugs/features we are fixing/adding. I agree it is a debatable whether it is a bug fix or a feature improvement and I am fine either way.
Lawrence
On 7/24/18, 10:13 AM, "Neal Cardwell" <ncardwell@google.com> wrote:
On Tue, Jul 24, 2018 at 1:07 PM Yuchung Cheng <ycheng@google.com> wrote:
>
> On Mon, Jul 23, 2018 at 7:23 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Should this go to net tree instead where all the other fixes went?
> I am neutral but this feels more like a feature improvement
I agree this feels like a feature improvement rather than a bug fix.
neal
^ permalink raw reply
* Re: [PATCHv3 net-next 2/2] selftests: add a selftest for directed broadcast forwarding
From: Xin Long @ 2018-07-24 17:55 UTC (permalink / raw)
To: David Ahern; +Cc: network dev, davem, Davide Caratti, Ido Schimmel
In-Reply-To: <ae804f04-2be2-e79d-5f43-033e275fbd66@gmail.com>
On Wed, Jul 25, 2018 at 1:37 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/24/18 11:24 AM, Xin Long wrote:
>> On Mon, Jul 23, 2018 at 11:17 PM, David Ahern <dsahern@gmail.com> wrote:
>>> On 7/23/18 5:51 AM, Xin Long wrote:
>>>> +ping_ipv4()
>>>> +{
>>>> + sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
>>>> +
>>>> + bc_forwarding_disable
>>>> + ping_test_from $h1 198.51.100.255 192.0.2.1
>>>> + ping_test_from $h1 198.51.200.255 192.0.2.1
>>>> + ping_test_from $h1 192.0.2.255 192.0.2.1
>>>> + ping_test_from $h1 255.255.255.255 192.0.2.1
>>>> +
>>>> + ping_test_from $h2 192.0.2.255 198.51.100.1
>>>> + ping_test_from $h2 198.51.200.255 198.51.100.1
>>>> + ping_test_from $h2 198.51.100.255 198.51.100.1
>>>> + ping_test_from $h2 255.255.255.255 198.51.100.1
>>>> + bc_forwarding_restore
>>>> +
>>>> + bc_forwarding_enable
>>>> + ping_test_from $h1 198.51.100.255 198.51.100.2
>>>> + ping_test_from $h1 198.51.200.255 198.51.200.2
>>>> + ping_test_from $h1 192.0.2.255 192.0.2.1 1
>>>> + ping_test_from $h1 255.255.255.255 192.0.2.1
>>>> +
>>>> + ping_test_from $h2 192.0.2.255 192.0.2.2
>>>> + ping_test_from $h2 198.51.200.255 198.51.200.2
>>>> + ping_test_from $h2 198.51.100.255 198.51.100.1 1
>>>> + ping_test_from $h2 255.255.255.255 198.51.100.1
>>>> + bc_forwarding_restore
>>>> +
>>>> + sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
>>>
>>> You need a better description for each test. This output:
>>> TEST: ping_test_from [PASS]
>>> TEST: ping_test_from [PASS]
>>> TEST: ping_test_from [PASS]
>>> TEST: ping_test_from [PASS]
>>> ...
>>>
>>> does not help in understanding which cases are working and which are not.
>> # ./router_broadcast.sh
>> INFO: bc_forwarding disabled on r1=>
>> INFO: h1 -> net2: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h1 -> net3: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h1 -> net1: reply from r1 (not dropping)
>> TEST: ping_test_from [PASS]
>> INFO: h1 -> 255.255.255.255: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> net1: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> net3: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> net2: reply from r1 (not dropping)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> 255.255.255.255: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: bc_forwarding enabled on r1 =>
>> INFO: h1 -> net2: reply from h2 (forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h1 -> net3: reply from h3 (forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h1 -> net1: no reply (dropping)
>> TEST: ping_test_from [PASS]
>> INFO: h1 -> 255.255.255.255: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> net1: reply from h3 (forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> net3: reply from h1 (forwarding)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> net2: no reply (dropping)
>> TEST: ping_test_from [PASS]
>> INFO: h2 -> 255.255.255.255: reply from r1 (not forwarding)
>> TEST: ping_test_from [PASS]
>>
>> I hope this log looks good to you?
>>
>
> The extra INFO is good, but the TEST line needs a better description.
>
INFO: bc_forwarding disabled on r1 =>
INFO: h1 -> net2: reply from r1 (not forwarding)
TEST: ping 198.51.100.255, expected reply from 192.0.2.1 [PASS]
INFO: h1 -> net3: reply from r1 (not forwarding)
TEST: ping 198.51.200.255, expected reply from 192.0.2.1 [PASS]
INFO: h1 -> net1: reply from r1 (not dropping)
TEST: ping 192.0.2.255, expected reply from 192.0.2.1 [PASS]
....
how about this?
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 01/10] audit: add container id
From: Richard Guy Briggs @ 2018-07-24 19:06 UTC (permalink / raw)
To: Paul Moore
Cc: simo, jlayton, carlos, linux-api, containers, linux-kernel,
Eric Paris, dhowells, linux-audit, ebiederm, luto, netdev,
linux-fsdevel, cgroups, serge, viro
In-Reply-To: <CAHC9VhRk+qLAyizM9i8D+cjKxyhruuj_Z9N0QRmjM-fjm4hgRw@mail.gmail.com>
On 2018-07-20 18:13, Paul Moore wrote:
> On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Implement the proc fs write to set the audit container identifier of a
> > process, emitting an AUDIT_CONTAINER_ID record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/audit_containerid where PID is the process ID of the
> > newly created task that is to become the first task in a container, or
> > an additional task added to a container.
> >
> > The write expects up to a u64 value (unset: 18446744073709551615).
> >
> > The writer must have capability CAP_AUDIT_CONTROL.
> >
> > This will produce a record such as this:
> > type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
> >
> > The "op" field indicates an initial set. The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained". Old and new audit container identifier values are
> > given in the "contid" fields, while res indicates its success.
> >
> > It is not permitted to unset or re-set the audit container identifier.
> > A child inherits its parent's audit container identifier, but then can
> > be set only once after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/90
> > See: https://github.com/linux-audit/audit-userspace/issues/51
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > fs/proc/base.c | 37 ++++++++++++++++++++++++
> > include/linux/audit.h | 25 ++++++++++++++++
> > include/uapi/linux/audit.h | 2 ++
> > kernel/auditsc.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 135 insertions(+)
>
> ...
>
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -606,6 +621,16 @@ static inline bool audit_loginuid_set(struct task_struct *tsk)
> > return uid_valid(audit_get_loginuid(tsk));
> > }
> >
> > +static inline bool cid_valid(u64 contid)
> > +{
> > + return contid != AUDIT_CID_UNSET;
> > +}
> > +
> > +static inline bool audit_contid_set(struct task_struct *tsk)
> > +{
> > + return cid_valid(audit_get_contid(tsk));
> > +}
>
> For the sake of consistency I think we should rename cid_valid() to
> audit_contid_valid().
Ok.
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 59ef7a81..611e926 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -956,6 +956,8 @@ int audit_alloc(struct task_struct *tsk)
> > return -ENOMEM;
> > info->loginuid = audit_get_loginuid(current);
> > info->sessionid = audit_get_sessionid(current);
> > + info->contid = audit_get_contid(current);
> > + info->inherited = true;
>
> First see my others comments in this patch about inheritence, but if
> we decide that flagging inherited values is important we should
> probably rename the "inherited" field to indicate that it applies to
> just the "contid" field.
Ok.
> > tsk->audit = info;
> >
> > if (likely(!audit_ever_enabled))
> > @@ -985,6 +987,8 @@ int audit_alloc(struct task_struct *tsk)
> > struct audit_task_info init_struct_audit = {
> > .loginuid = INVALID_UID,
> > .sessionid = AUDIT_SID_UNSET,
> > + .contid = AUDIT_CID_UNSET,
> > + .inherited = true,
> > .ctx = NULL,
> > };
> >
> > @@ -2112,6 +2116,73 @@ int audit_set_loginuid(kuid_t loginuid)
> > }
> >
> > /**
> > + * audit_set_contid - set current task's audit_context contid
> > + * @contid: contid value
> > + *
> > + * Returns 0 on success, -EPERM on permission failure.
> > + *
> > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > + */
> > +int audit_set_contid(struct task_struct *task, u64 contid)
> > +{
> > + u64 oldcontid;
> > + int rc = 0;
> > + struct audit_buffer *ab;
> > + uid_t uid;
> > + struct tty_struct *tty;
> > + char comm[sizeof(current->comm)];
> > +
> > + /* Can't set if audit disabled */
> > + if (!task->audit)
> > + return -ENOPROTOOPT;
> > + oldcontid = audit_get_contid(task);
> > + /* Don't allow the audit containerid to be unset */
> > + if (!cid_valid(contid))
> > + rc = -EINVAL;
> > + /* if we don't have caps, reject */
> > + else if (!capable(CAP_AUDIT_CONTROL))
> > + rc = -EPERM;
> > + /* if task has children or is not single-threaded, deny */
> > + else if (!list_empty(&task->children))
> > + rc = -EBUSY;
>
> Is this safe without holding tasklist_lock? I worry we might be
> vulnerable to a race with fork().
>
> > + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > + rc = -EALREADY;
>
> Similar concern here as well, although related to threads.
I think you are correct here and tasklist_lock should cover both. Do we
also want rcu_read_lock() immediately preceeding it?
> > + /* it is already set, and not inherited from the parent, reject */
> > + else if (cid_valid(oldcontid) && !task->audit->inherited)
> > + rc = -EEXIST;
>
> Maybe I'm missing something, but why do we care about preventing
> reassigning the audit container ID in this case? The task is single
> threaded and has no descendants at this point so it should be safe,
> yes? So long as the task changing the audit container ID has
> capable(CAP_AUDIT_CONTOL) it shouldn't matter, right?
Because we hammered out this idea 6 months ago in the design phase and I
thought we all firmly agreed that the audit container identifier could
only be set once. Has any significant discussion happenned since then
to change that wisdom? I just wonder why this is coming up now.
> Related, I'm questioning if we would ever care if the audit container
> ID was inherited or not?
We do since that is the only way we can tell if the value has been set
once already or inherited unless we check if the parent's audit
container identifier is identical (which tells us it was inherited).
> > + if (!rc) {
> > + task_lock(task);
> > + task->audit->contid = contid;
> > + task->audit->inherited = false;
> > + task_unlock(task);
>
> I suspect the task_lock() may not be what we want here, but if we are
> using task_lock() to protect the audit fields two things come to mind:
>
> 1. We should update the header comments for task_lock() in task.h to
> indicate that it also protects ->audit.
Fair enough.
> 2. Where else do we need to worry about taking this lock? At the very
> least we should take this lock near the top of this function before we
> check task->audit and not drop it until after we have set it, or
> failed the operation for one of the reasons above.
Agreed, since another process on another CPU could race attempting this
same operation. However, the task_lock() comment precludes using it
with write_lock_irq(&task_lock) that might be required above.
> > + }
> > +
> > + if (!audit_enabled)
> > + return rc;
> > +
> > + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_ID);
> > + if (!ab)
> > + return rc;
> > +
> > + uid = from_kuid(&init_user_ns, task_uid(current));
> > + tty = audit_get_tty(current);
> > + audit_log_format(ab, "op=set opid=%d old-contid=%llu contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
> > + task_tgid_nr(task), oldcontid, contid,
> > + task_tgid_nr(current), uid
> > + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > + tty ? tty_name(tty) : "(none)",
> > + audit_get_sessionid(current));
> > + audit_put_tty(tty);
> > + audit_log_task_context(ab);
> > + audit_log_format(ab, " comm=");
> > + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > + audit_log_d_path_exe(ab, current->mm);
> > + audit_log_format(ab, " res=%d", !rc);
> > + audit_log_end(ab);
> > + return rc;
> > +}
> > +
> > +/**
> > * __audit_mq_open - record audit data for a POSIX MQ open
> > * @oflag: open flag
> > * @mode: mode bits
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox