* [PATCH net-next 08/17] rxrpc: Don't leak the service-side session key to userspace
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Don't let someone reading a service-side rxrpc-type key get access to the
session key that was exchanged with the client. The server application
will, at some point, need to be able to read the information in the ticket,
but this probably shouldn't include the key material.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/keys/rxrpc-type.h | 1 +
net/rxrpc/key.c | 8 ++++++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/keys/rxrpc-type.h b/include/keys/rxrpc-type.h
index 8e4ced9b4ecf..333c0f49a9cd 100644
--- a/include/keys/rxrpc-type.h
+++ b/include/keys/rxrpc-type.h
@@ -36,6 +36,7 @@ struct rxkad_key {
*/
struct rxrpc_key_token {
u16 security_index; /* RxRPC header security index */
+ bool no_leak_key; /* Don't copy the key to userspace */
struct rxrpc_key_token *next; /* the next token in the list */
union {
struct rxkad_key *kad;
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 3bd7b9d48d27..ed29ec01237b 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -579,7 +579,8 @@ static long rxrpc_read(const struct key *key,
case RXRPC_SECURITY_RXKAD:
toksize += 8 * 4; /* viceid, kvno, key*2, begin,
* end, primary, tktlen */
- toksize += RND(token->kad->ticket_len);
+ if (!token->no_leak_key)
+ toksize += RND(token->kad->ticket_len);
break;
default: /* we have a ticket we can't encode */
@@ -654,7 +655,10 @@ static long rxrpc_read(const struct key *key,
ENCODE(token->kad->start);
ENCODE(token->kad->expiry);
ENCODE(token->kad->primary_flag);
- ENCODE_DATA(token->kad->ticket_len, token->kad->ticket);
+ if (token->no_leak_key)
+ ENCODE(0);
+ else
+ ENCODE_DATA(token->kad->ticket_len, token->kad->ticket);
break;
default:
^ permalink raw reply related
* [PATCH net-next 10/17] rxrpc: Make the parsing of xdr payloads more coherent
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Make the parsing of xdr-encoded payloads, as passed to add_key, more
coherent. Shuttling back and forth between various variables was a bit
hard to follow.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/key.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index ed29ec01237b..a9d8f5b466be 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -135,7 +135,7 @@ static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep,
*/
static int rxrpc_preparse_xdr(struct key_preparsed_payload *prep)
{
- const __be32 *xdr = prep->data, *token;
+ const __be32 *xdr = prep->data, *token, *p;
const char *cp;
unsigned int len, paddedlen, loop, ntoken, toklen, sec_ix;
size_t datalen = prep->datalen;
@@ -189,20 +189,20 @@ static int rxrpc_preparse_xdr(struct key_preparsed_payload *prep)
goto not_xdr;
/* check each token wrapper */
- token = xdr;
+ p = xdr;
loop = ntoken;
do {
if (datalen < 8)
goto not_xdr;
- toklen = ntohl(*xdr++);
- sec_ix = ntohl(*xdr);
+ toklen = ntohl(*p++);
+ sec_ix = ntohl(*p);
datalen -= 4;
_debug("token: [%x/%zx] %x", toklen, datalen, sec_ix);
paddedlen = (toklen + 3) & ~3;
if (toklen < 20 || toklen > datalen || paddedlen > datalen)
goto not_xdr;
datalen -= paddedlen;
- xdr += paddedlen >> 2;
+ p += paddedlen >> 2;
} while (--loop > 0);
@@ -214,17 +214,18 @@ static int rxrpc_preparse_xdr(struct key_preparsed_payload *prep)
* - we ignore the cellname, relying on the key to be correctly named
*/
do {
- xdr = token;
toklen = ntohl(*xdr++);
- token = xdr + ((toklen + 3) >> 2);
- sec_ix = ntohl(*xdr++);
+ token = xdr;
+ xdr += (toklen + 3) / 4;
+
+ sec_ix = ntohl(*token++);
toklen -= 4;
- _debug("TOKEN type=%u [%p-%p]", sec_ix, xdr, token);
+ _debug("TOKEN type=%x len=%x", sec_ix, toklen);
switch (sec_ix) {
case RXRPC_SECURITY_RXKAD:
- ret = rxrpc_preparse_xdr_rxkad(prep, datalen, xdr, toklen);
+ ret = rxrpc_preparse_xdr_rxkad(prep, datalen, token, toklen);
if (ret != 0)
goto error;
break;
^ permalink raw reply related
* [PATCH net-next 11/17] rxrpc: Ignore unknown tokens in key payload unless no known tokens
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
When parsing a payload for an rxrpc-type key, ignore any tokens that are
not of a known type and don't give an error for them - unless there are no
tokens of a known type.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/key.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index a9d8f5b466be..7e6d19263ce3 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -139,7 +139,7 @@ static int rxrpc_preparse_xdr(struct key_preparsed_payload *prep)
const char *cp;
unsigned int len, paddedlen, loop, ntoken, toklen, sec_ix;
size_t datalen = prep->datalen;
- int ret;
+ int ret, ret2;
_enter(",{%x,%x,%x,%x},%zu",
ntohl(xdr[0]), ntohl(xdr[1]), ntohl(xdr[2]), ntohl(xdr[3]),
@@ -213,6 +213,7 @@ static int rxrpc_preparse_xdr(struct key_preparsed_payload *prep)
/* okay: we're going to assume it's valid XDR format
* - we ignore the cellname, relying on the key to be correctly named
*/
+ ret = -EPROTONOSUPPORT;
do {
toklen = ntohl(*xdr++);
token = xdr;
@@ -225,27 +226,37 @@ static int rxrpc_preparse_xdr(struct key_preparsed_payload *prep)
switch (sec_ix) {
case RXRPC_SECURITY_RXKAD:
- ret = rxrpc_preparse_xdr_rxkad(prep, datalen, token, toklen);
- if (ret != 0)
- goto error;
+ ret2 = rxrpc_preparse_xdr_rxkad(prep, datalen, token, toklen);
+ break;
+ default:
+ ret2 = -EPROTONOSUPPORT;
break;
+ }
+ switch (ret2) {
+ case 0:
+ ret = 0;
+ break;
+ case -EPROTONOSUPPORT:
+ break;
+ case -ENOPKG:
+ if (ret != 0)
+ ret = -ENOPKG;
+ break;
default:
- ret = -EPROTONOSUPPORT;
+ ret = ret2;
goto error;
}
} while (--ntoken > 0);
- _leave(" = 0");
- return 0;
+error:
+ _leave(" = %d", ret);
+ return ret;
not_xdr:
_leave(" = -EPROTO");
return -EPROTO;
-error:
- _leave(" = %d", ret);
- return ret;
}
/*
^ permalink raw reply related
* [PATCH net-next 09/17] rxrpc: Allow security classes to give more info on server keys
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Allow a security class to give more information on an rxrpc_s-type key when
it is viewed in /proc/keys. This will allow the upcoming RxGK security
class to show the enctype name here.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 3 +++
net/rxrpc/server_key.c | 4 ++++
2 files changed, 7 insertions(+)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 6682c797b878..0fb294725ff2 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -227,6 +227,9 @@ struct rxrpc_security {
/* Destroy the payload of a server key */
void (*destroy_server_key)(struct key *);
+ /* Describe a server key */
+ void (*describe_server_key)(const struct key *, struct seq_file *);
+
/* initialise a connection's security */
int (*init_connection_security)(struct rxrpc_connection *,
struct rxrpc_key_token *);
diff --git a/net/rxrpc/server_key.c b/net/rxrpc/server_key.c
index 1a2f0b63ee1d..ead3471307ee 100644
--- a/net/rxrpc/server_key.c
+++ b/net/rxrpc/server_key.c
@@ -105,7 +105,11 @@ static void rxrpc_destroy_s(struct key *key)
static void rxrpc_describe_s(const struct key *key, struct seq_file *m)
{
+ const struct rxrpc_security *sec = key->payload.data[1];
+
seq_puts(m, key->description);
+ if (sec && sec->describe_server_key)
+ sec->describe_server_key(key, m);
}
/*
^ permalink raw reply related
* [PATCH net-next 12/17] rxrpc: Fix example key name in a comment
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Fix an example of an rxrpc key name in a comment.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/key.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 7e6d19263ce3..9631aa8543b5 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -5,7 +5,7 @@
* Written by David Howells (dhowells@redhat.com)
*
* RxRPC keys should have a description of describing their purpose:
- * "afs@CAMBRIDGE.REDHAT.COM>
+ * "afs@example.com"
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
^ permalink raw reply related
* [PATCH net-next 13/17] rxrpc: Merge prime_packet_security into init_connection_security
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Merge the ->prime_packet_security() into the ->init_connection_security()
hook as they're always called together.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 2 --
net/rxrpc/conn_client.c | 6 ------
net/rxrpc/conn_event.c | 4 ----
net/rxrpc/insecure.c | 6 ------
net/rxrpc/rxkad.c | 20 +++++++++++++++-----
5 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 0fb294725ff2..6aaa0f49dab0 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -234,8 +234,6 @@ struct rxrpc_security {
int (*init_connection_security)(struct rxrpc_connection *,
struct rxrpc_key_token *);
- /* prime a connection's packet security */
- int (*prime_packet_security)(struct rxrpc_connection *);
/* impose security on a packet */
int (*secure_packet)(struct rxrpc_call *,
diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index 7e574c75be8e..dbea0bfee48e 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -180,10 +180,6 @@ rxrpc_alloc_client_connection(struct rxrpc_bundle *bundle, gfp_t gfp)
if (ret < 0)
goto error_1;
- ret = conn->security->prime_packet_security(conn);
- if (ret < 0)
- goto error_2;
-
atomic_inc(&rxnet->nr_conns);
write_lock(&rxnet->conn_lock);
list_add_tail(&conn->proc_link, &rxnet->conn_proc_list);
@@ -203,8 +199,6 @@ rxrpc_alloc_client_connection(struct rxrpc_bundle *bundle, gfp_t gfp)
_leave(" = %p", conn);
return conn;
-error_2:
- conn->security->clear(conn);
error_1:
rxrpc_put_client_connection_id(conn);
error_0:
diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c
index 03a482ba770f..aab069701398 100644
--- a/net/rxrpc/conn_event.c
+++ b/net/rxrpc/conn_event.c
@@ -338,10 +338,6 @@ static int rxrpc_process_event(struct rxrpc_connection *conn,
if (ret < 0)
return ret;
- ret = conn->security->prime_packet_security(conn);
- if (ret < 0)
- return ret;
-
spin_lock(&conn->bundle->channel_lock);
spin_lock_bh(&conn->state_lock);
diff --git a/net/rxrpc/insecure.c b/net/rxrpc/insecure.c
index cf3ecffcf424..914e2f2e2990 100644
--- a/net/rxrpc/insecure.c
+++ b/net/rxrpc/insecure.c
@@ -14,11 +14,6 @@ static int none_init_connection_security(struct rxrpc_connection *conn,
return 0;
}
-static int none_prime_packet_security(struct rxrpc_connection *conn)
-{
- return 0;
-}
-
static int none_secure_packet(struct rxrpc_call *call,
struct sk_buff *skb,
size_t data_size,
@@ -87,7 +82,6 @@ const struct rxrpc_security rxrpc_no_security = {
.init = none_init,
.exit = none_exit,
.init_connection_security = none_init_connection_security,
- .prime_packet_security = none_prime_packet_security,
.free_call_crypto = none_free_call_crypto,
.secure_packet = none_secure_packet,
.verify_packet = none_verify_packet,
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 3057f00a6978..301894857473 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -38,6 +38,9 @@ struct rxkad_level2_hdr {
__be32 checksum; /* decrypted data checksum */
};
+static int rxkad_prime_packet_security(struct rxrpc_connection *conn,
+ struct crypto_sync_skcipher *ci);
+
/*
* this holds a pinned cipher so that keventd doesn't get called by the cipher
* alloc routine, but since we have it to hand, we use it to decrypt RESPONSE
@@ -130,8 +133,15 @@ static int rxkad_init_connection_security(struct rxrpc_connection *conn,
goto error;
}
+ ret = rxkad_prime_packet_security(conn, ci);
+ if (ret < 0)
+ goto error_ci;
+
conn->cipher = ci;
- ret = 0;
+ return 0;
+
+error_ci:
+ crypto_free_sync_skcipher(ci);
error:
_leave(" = %d", ret);
return ret;
@@ -141,7 +151,8 @@ static int rxkad_init_connection_security(struct rxrpc_connection *conn,
* prime the encryption state with the invariant parts of a connection's
* description
*/
-static int rxkad_prime_packet_security(struct rxrpc_connection *conn)
+static int rxkad_prime_packet_security(struct rxrpc_connection *conn,
+ struct crypto_sync_skcipher *ci)
{
struct skcipher_request *req;
struct rxrpc_key_token *token;
@@ -159,7 +170,7 @@ static int rxkad_prime_packet_security(struct rxrpc_connection *conn)
if (!tmpbuf)
return -ENOMEM;
- req = skcipher_request_alloc(&conn->cipher->base, GFP_NOFS);
+ req = skcipher_request_alloc(&ci->base, GFP_NOFS);
if (!req) {
kfree(tmpbuf);
return -ENOMEM;
@@ -174,7 +185,7 @@ static int rxkad_prime_packet_security(struct rxrpc_connection *conn)
tmpbuf[3] = htonl(conn->security_ix);
sg_init_one(&sg, tmpbuf, tmpsize);
- skcipher_request_set_sync_tfm(req, conn->cipher);
+ skcipher_request_set_sync_tfm(req, ci);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, &sg, &sg, tmpsize, iv.x);
crypto_skcipher_encrypt(req);
@@ -1350,7 +1361,6 @@ const struct rxrpc_security rxkad = {
.free_preparse_server_key = rxkad_free_preparse_server_key,
.destroy_server_key = rxkad_destroy_server_key,
.init_connection_security = rxkad_init_connection_security,
- .prime_packet_security = rxkad_prime_packet_security,
.secure_packet = rxkad_secure_packet,
.verify_packet = rxkad_verify_packet,
.free_call_crypto = rxkad_free_call_crypto,
^ permalink raw reply related
* [PATCH net-next 14/17] rxrpc: Don't reserve security header in Tx DATA skbuff
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Insert the security header into the skbuff representing a DATA packet to be
transmitted rather than using skb_reserve() when the packet is allocated.
This makes it easier to apply crypto that spans the security header and the
data, particularly in the upcoming RxGK class where we have a common
encrypt-and-checksum function that is used in a number of circumstances.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 5 +----
net/rxrpc/insecure.c | 6 ++----
net/rxrpc/rxkad.c | 24 +++++++++---------------
net/rxrpc/sendmsg.c | 6 ++----
4 files changed, 14 insertions(+), 27 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 6aaa0f49dab0..742a69fb8e60 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -236,10 +236,7 @@ struct rxrpc_security {
/* impose security on a packet */
- int (*secure_packet)(struct rxrpc_call *,
- struct sk_buff *,
- size_t,
- void *);
+ int (*secure_packet)(struct rxrpc_call *, struct sk_buff *, size_t);
/* verify the security on a received packet */
int (*verify_packet)(struct rxrpc_call *, struct sk_buff *,
diff --git a/net/rxrpc/insecure.c b/net/rxrpc/insecure.c
index 914e2f2e2990..e06725e21c05 100644
--- a/net/rxrpc/insecure.c
+++ b/net/rxrpc/insecure.c
@@ -14,10 +14,8 @@ static int none_init_connection_security(struct rxrpc_connection *conn,
return 0;
}
-static int none_secure_packet(struct rxrpc_call *call,
- struct sk_buff *skb,
- size_t data_size,
- void *sechdr)
+static int none_secure_packet(struct rxrpc_call *call, struct sk_buff *skb,
+ size_t data_size)
{
return 0;
}
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 301894857473..37335d887570 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -230,9 +230,7 @@ static void rxkad_free_call_crypto(struct rxrpc_call *call)
* partially encrypt a packet (level 1 security)
*/
static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
- struct sk_buff *skb,
- u32 data_size,
- void *sechdr,
+ struct sk_buff *skb, u32 data_size,
struct skcipher_request *req)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
@@ -247,12 +245,12 @@ static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
data_size |= (u32)check << 16;
hdr.data_size = htonl(data_size);
- memcpy(sechdr, &hdr, sizeof(hdr));
+ memcpy(skb->head, &hdr, sizeof(hdr));
/* start the encryption afresh */
memset(&iv, 0, sizeof(iv));
- sg_init_one(&sg, sechdr, 8);
+ sg_init_one(&sg, skb->head, 8);
skcipher_request_set_sync_tfm(req, call->conn->cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, &sg, &sg, 8, iv.x);
@@ -269,7 +267,6 @@ 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,
struct skcipher_request *req)
{
const struct rxrpc_key_token *token;
@@ -289,13 +286,13 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
rxkhdr.data_size = htonl(data_size | (u32)check << 16);
rxkhdr.checksum = 0;
- memcpy(sechdr, &rxkhdr, sizeof(rxkhdr));
+ memcpy(skb->head, &rxkhdr, sizeof(rxkhdr));
/* encrypt from the session key */
token = call->conn->params.key->payload.data[0];
memcpy(&iv, token->kad->session_key, sizeof(iv));
- sg_init_one(&sg[0], sechdr, sizeof(rxkhdr));
+ sg_init_one(&sg[0], skb->head, sizeof(rxkhdr));
skcipher_request_set_sync_tfm(req, call->conn->cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, &sg[0], &sg[0], sizeof(rxkhdr), iv.x);
@@ -310,7 +307,7 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
len &= ~(call->conn->size_align - 1);
sg_init_table(sg, ARRAY_SIZE(sg));
- err = skb_to_sgvec(skb, sg, 0, len);
+ err = skb_to_sgvec(skb, sg, 8, len);
if (unlikely(err < 0))
goto out;
skcipher_request_set_crypt(req, sg, sg, len, iv.x);
@@ -329,8 +326,7 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
*/
static int rxkad_secure_packet(struct rxrpc_call *call,
struct sk_buff *skb,
- size_t data_size,
- void *sechdr)
+ size_t data_size)
{
struct rxrpc_skb_priv *sp;
struct skcipher_request *req;
@@ -383,12 +379,10 @@ 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,
- req);
+ ret = rxkad_secure_packet_auth(call, skb, data_size, req);
break;
case RXRPC_SECURITY_ENCRYPT:
- ret = rxkad_secure_packet_encrypt(call, skb, data_size,
- sechdr, req);
+ ret = rxkad_secure_packet_encrypt(call, skb, data_size, req);
break;
default:
ret = -EPERM;
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index d27140c836cc..367654a558c2 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -372,8 +372,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
ASSERTCMP(skb->mark, ==, 0);
_debug("HS: %u", call->conn->security_size);
- skb_reserve(skb, call->conn->security_size);
- skb->len += call->conn->security_size;
+ __skb_put(skb, call->conn->security_size);
sp->remain = chunk;
if (sp->remain > skb_tailroom(skb))
@@ -446,8 +445,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
call->tx_winsize)
sp->hdr.flags |= RXRPC_MORE_PACKETS;
- ret = call->security->secure_packet(
- call, skb, skb->mark, skb->head);
+ ret = call->security->secure_packet(call, skb, skb->mark);
if (ret < 0)
goto out;
^ permalink raw reply related
* [PATCH net-next 15/17] rxrpc: Organise connection security to use a union
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Organise the security information in the rxrpc_connection struct to use a
union to allow for different data for different security classes.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 11 ++++++++---
net/rxrpc/rxkad.c | 40 ++++++++++++++++++++--------------------
2 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 742a69fb8e60..fda6618df1cc 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -448,9 +448,15 @@ struct rxrpc_connection {
struct list_head proc_link; /* link in procfs list */
struct list_head link; /* link in master connection list */
struct sk_buff_head rx_queue; /* received conn-level packets */
+
const struct rxrpc_security *security; /* applied security module */
- struct crypto_sync_skcipher *cipher; /* encryption handle */
- struct rxrpc_crypt csum_iv; /* packet checksum base */
+ union {
+ struct {
+ struct crypto_sync_skcipher *cipher; /* encryption handle */
+ struct rxrpc_crypt csum_iv; /* packet checksum base */
+ u32 nonce; /* response re-use preventer */
+ } rxkad;
+ };
unsigned long flags;
unsigned long events;
unsigned long idle_timestamp; /* Time at which last became idle */
@@ -460,7 +466,6 @@ struct rxrpc_connection {
int debug_id; /* debug ID for printks */
atomic_t serial; /* packet serial number counter */
unsigned int hi_serial; /* highest serial number received */
- u32 security_nonce; /* response re-use preventer */
u32 service_id; /* Service ID, possibly upgraded */
u8 size_align; /* data size alignment (for security) */
u8 security_size; /* security header size */
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 37335d887570..f3182edfcbae 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -137,7 +137,7 @@ static int rxkad_init_connection_security(struct rxrpc_connection *conn,
if (ret < 0)
goto error_ci;
- conn->cipher = ci;
+ conn->rxkad.cipher = ci;
return 0;
error_ci:
@@ -191,7 +191,7 @@ static int rxkad_prime_packet_security(struct rxrpc_connection *conn,
crypto_skcipher_encrypt(req);
skcipher_request_free(req);
- memcpy(&conn->csum_iv, tmpbuf + 2, sizeof(conn->csum_iv));
+ memcpy(&conn->rxkad.csum_iv, tmpbuf + 2, sizeof(conn->rxkad.csum_iv));
kfree(tmpbuf);
_leave(" = 0");
return 0;
@@ -203,7 +203,7 @@ static int rxkad_prime_packet_security(struct rxrpc_connection *conn,
*/
static struct skcipher_request *rxkad_get_call_crypto(struct rxrpc_call *call)
{
- struct crypto_skcipher *tfm = &call->conn->cipher->base;
+ struct crypto_skcipher *tfm = &call->conn->rxkad.cipher->base;
struct skcipher_request *cipher_req = call->cipher_req;
if (!cipher_req) {
@@ -251,7 +251,7 @@ static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
memset(&iv, 0, sizeof(iv));
sg_init_one(&sg, skb->head, 8);
- skcipher_request_set_sync_tfm(req, call->conn->cipher);
+ skcipher_request_set_sync_tfm(req, call->conn->rxkad.cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, &sg, &sg, 8, iv.x);
crypto_skcipher_encrypt(req);
@@ -293,7 +293,7 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
memcpy(&iv, token->kad->session_key, sizeof(iv));
sg_init_one(&sg[0], skb->head, sizeof(rxkhdr));
- skcipher_request_set_sync_tfm(req, call->conn->cipher);
+ skcipher_request_set_sync_tfm(req, call->conn->rxkad.cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, &sg[0], &sg[0], sizeof(rxkhdr), iv.x);
crypto_skcipher_encrypt(req);
@@ -341,7 +341,7 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
call->debug_id, key_serial(call->conn->params.key),
sp->hdr.seq, data_size);
- if (!call->conn->cipher)
+ if (!call->conn->rxkad.cipher)
return 0;
ret = key_validate(call->conn->params.key);
@@ -353,7 +353,7 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
return -ENOMEM;
/* continue encrypting from where we left off */
- memcpy(&iv, call->conn->csum_iv.x, sizeof(iv));
+ memcpy(&iv, call->conn->rxkad.csum_iv.x, sizeof(iv));
/* calculate the security checksum */
x = (call->cid & RXRPC_CHANNELMASK) << (32 - RXRPC_CIDSHIFT);
@@ -362,7 +362,7 @@ static int rxkad_secure_packet(struct rxrpc_call *call,
call->crypto_buf[1] = htonl(x);
sg_init_one(&sg, call->crypto_buf, 8);
- skcipher_request_set_sync_tfm(req, call->conn->cipher);
+ skcipher_request_set_sync_tfm(req, call->conn->rxkad.cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, &sg, &sg, 8, iv.x);
crypto_skcipher_encrypt(req);
@@ -428,7 +428,7 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
/* start the decryption afresh */
memset(&iv, 0, sizeof(iv));
- skcipher_request_set_sync_tfm(req, call->conn->cipher);
+ skcipher_request_set_sync_tfm(req, call->conn->rxkad.cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, sg, sg, 8, iv.x);
crypto_skcipher_decrypt(req);
@@ -520,7 +520,7 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
token = call->conn->params.key->payload.data[0];
memcpy(&iv, token->kad->session_key, sizeof(iv));
- skcipher_request_set_sync_tfm(req, call->conn->cipher);
+ skcipher_request_set_sync_tfm(req, call->conn->rxkad.cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, sg, sg, len, iv.x);
crypto_skcipher_decrypt(req);
@@ -586,7 +586,7 @@ static int rxkad_verify_packet(struct rxrpc_call *call, struct sk_buff *skb,
_enter("{%d{%x}},{#%u}",
call->debug_id, key_serial(call->conn->params.key), seq);
- if (!call->conn->cipher)
+ if (!call->conn->rxkad.cipher)
return 0;
req = rxkad_get_call_crypto(call);
@@ -594,7 +594,7 @@ static int rxkad_verify_packet(struct rxrpc_call *call, struct sk_buff *skb,
return -ENOMEM;
/* continue encrypting from where we left off */
- memcpy(&iv, call->conn->csum_iv.x, sizeof(iv));
+ memcpy(&iv, call->conn->rxkad.csum_iv.x, sizeof(iv));
/* validate the security checksum */
x = (call->cid & RXRPC_CHANNELMASK) << (32 - RXRPC_CIDSHIFT);
@@ -603,7 +603,7 @@ static int rxkad_verify_packet(struct rxrpc_call *call, struct sk_buff *skb,
call->crypto_buf[1] = htonl(x);
sg_init_one(&sg, call->crypto_buf, 8);
- skcipher_request_set_sync_tfm(req, call->conn->cipher);
+ skcipher_request_set_sync_tfm(req, call->conn->rxkad.cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, &sg, &sg, 8, iv.x);
crypto_skcipher_encrypt(req);
@@ -698,10 +698,10 @@ static int rxkad_issue_challenge(struct rxrpc_connection *conn)
_enter("{%d}", conn->debug_id);
- get_random_bytes(&conn->security_nonce, sizeof(conn->security_nonce));
+ get_random_bytes(&conn->rxkad.nonce, sizeof(conn->rxkad.nonce));
challenge.version = htonl(2);
- challenge.nonce = htonl(conn->security_nonce);
+ challenge.nonce = htonl(conn->rxkad.nonce);
challenge.min_level = htonl(0);
challenge.__padding = 0;
@@ -829,7 +829,7 @@ static int rxkad_encrypt_response(struct rxrpc_connection *conn,
struct rxrpc_crypt iv;
struct scatterlist sg[1];
- req = skcipher_request_alloc(&conn->cipher->base, GFP_NOFS);
+ req = skcipher_request_alloc(&conn->rxkad.cipher->base, GFP_NOFS);
if (!req)
return -ENOMEM;
@@ -838,7 +838,7 @@ static int rxkad_encrypt_response(struct rxrpc_connection *conn,
sg_init_table(sg, 1);
sg_set_buf(sg, &resp->encrypted, sizeof(resp->encrypted));
- skcipher_request_set_sync_tfm(req, conn->cipher);
+ skcipher_request_set_sync_tfm(req, conn->rxkad.cipher);
skcipher_request_set_callback(req, 0, NULL, NULL);
skcipher_request_set_crypt(req, sg, sg, sizeof(resp->encrypted), iv.x);
crypto_skcipher_encrypt(req);
@@ -1249,7 +1249,7 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
eproto = tracepoint_string("rxkad_rsp_seq");
abort_code = RXKADOUTOFSEQUENCE;
- if (ntohl(response->encrypted.inc_nonce) != conn->security_nonce + 1)
+ if (ntohl(response->encrypted.inc_nonce) != conn->rxkad.nonce + 1)
goto protocol_error_free;
eproto = tracepoint_string("rxkad_rsp_level");
@@ -1302,8 +1302,8 @@ static void rxkad_clear(struct rxrpc_connection *conn)
{
_enter("");
- if (conn->cipher)
- crypto_free_sync_skcipher(conn->cipher);
+ if (conn->rxkad.cipher)
+ crypto_free_sync_skcipher(conn->rxkad.cipher);
}
/*
^ permalink raw reply related
* [PATCH net-next 16/17] rxrpc: rxkad: Don't use pskb_pull() to advance through the response packet
From: David Howells @ 2020-11-23 20:11 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
In the rxkad security class, don't use pskb_pull() to advance through the
contents of the response packet. There's no point, especially as the next
and last access to the skbuff still has to allow for the wire header in the
offset (which we didn't advance over).
Better to just add the displacement to the next offset.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/rxkad.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index f3182edfcbae..e5b4bbdd0f34 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -1162,8 +1162,6 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
if (skb_copy_bits(skb, sizeof(struct rxrpc_wire_header),
response, sizeof(*response)) < 0)
goto protocol_error;
- if (!pskb_pull(skb, sizeof(*response)))
- BUG();
version = ntohl(response->version);
ticket_len = ntohl(response->ticket_len);
@@ -1194,7 +1192,7 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
eproto = tracepoint_string("rxkad_tkt_short");
abort_code = RXKADPACKETSHORT;
- if (skb_copy_bits(skb, sizeof(struct rxrpc_wire_header),
+ if (skb_copy_bits(skb, sizeof(struct rxrpc_wire_header) + sizeof(*response),
ticket, ticket_len) < 0)
goto protocol_error_free;
^ permalink raw reply related
* [PATCH net-next 17/17] rxrpc: Ask the security class how much space to allow in a packet
From: David Howells @ 2020-11-23 20:12 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <160616220405.830164.2239716599743995145.stgit@warthog.procyon.org.uk>
Ask the security class how much header and trailer space to allow for when
allocating a packet, given how much data is remaining.
This will allow the rxgk security class to stick both a trailer in as well
as a header as appropriate in the future.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 7 ++++-
net/rxrpc/conn_object.c | 1 -
net/rxrpc/insecure.c | 12 +++++++++
net/rxrpc/rxkad.c | 61 ++++++++++++++++++++++++++++++++++++++++-------
net/rxrpc/sendmsg.c | 41 ++++++++++----------------------
5 files changed, 82 insertions(+), 40 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index fda6618df1cc..7bd6f8a66a3e 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -234,6 +234,11 @@ struct rxrpc_security {
int (*init_connection_security)(struct rxrpc_connection *,
struct rxrpc_key_token *);
+ /* Work out how much data we can store in a packet, given an estimate
+ * of the amount of data remaining.
+ */
+ int (*how_much_data)(struct rxrpc_call *, size_t,
+ size_t *, size_t *, size_t *);
/* impose security on a packet */
int (*secure_packet)(struct rxrpc_call *, struct sk_buff *, size_t);
@@ -467,8 +472,6 @@ struct rxrpc_connection {
atomic_t serial; /* packet serial number counter */
unsigned int hi_serial; /* highest serial number received */
u32 service_id; /* Service ID, possibly upgraded */
- u8 size_align; /* data size alignment (for security) */
- u8 security_size; /* security header size */
u8 security_ix; /* security type */
u8 out_clientflag; /* RXRPC_CLIENT_INITIATED if we are client */
u8 bundle_shift; /* Index into bundle->avail_chans */
diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 8dd1ef25b98f..b2159dbf5412 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -49,7 +49,6 @@ struct rxrpc_connection *rxrpc_alloc_connection(gfp_t gfp)
conn->security = &rxrpc_no_security;
spin_lock_init(&conn->state_lock);
conn->debug_id = atomic_inc_return(&rxrpc_debug_id);
- conn->size_align = 4;
conn->idle_timestamp = jiffies;
}
diff --git a/net/rxrpc/insecure.c b/net/rxrpc/insecure.c
index e06725e21c05..9aae99d67833 100644
--- a/net/rxrpc/insecure.c
+++ b/net/rxrpc/insecure.c
@@ -14,6 +14,17 @@ static int none_init_connection_security(struct rxrpc_connection *conn,
return 0;
}
+/*
+ * Work out how much data we can put in an unsecured packet.
+ */
+static int none_how_much_data(struct rxrpc_call *call, size_t remain,
+ size_t *_buf_size, size_t *_data_size, size_t *_offset)
+{
+ *_buf_size = *_data_size = min_t(size_t, remain, RXRPC_JUMBO_DATALEN);
+ *_offset = 0;
+ return 0;
+}
+
static int none_secure_packet(struct rxrpc_call *call, struct sk_buff *skb,
size_t data_size)
{
@@ -81,6 +92,7 @@ const struct rxrpc_security rxrpc_no_security = {
.exit = none_exit,
.init_connection_security = none_init_connection_security,
.free_call_crypto = none_free_call_crypto,
+ .how_much_data = none_how_much_data,
.secure_packet = none_secure_packet,
.verify_packet = none_verify_packet,
.locate_data = none_locate_data,
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index e5b4bbdd0f34..e2e9e9b0a6d7 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -28,6 +28,7 @@
#define INST_SZ 40 /* size of principal's instance */
#define REALM_SZ 40 /* size of principal's auth domain */
#define SNAME_SZ 40 /* size of service name */
+#define RXKAD_ALIGN 8
struct rxkad_level1_hdr {
__be32 data_size; /* true data size (excluding padding) */
@@ -80,7 +81,7 @@ static int rxkad_preparse_server_key(struct key_preparsed_payload *prep)
static void rxkad_free_preparse_server_key(struct key_preparsed_payload *prep)
{
-
+
if (prep->payload.data[0])
crypto_free_skcipher(prep->payload.data[0]);
}
@@ -119,14 +120,8 @@ static int rxkad_init_connection_security(struct rxrpc_connection *conn,
switch (conn->params.security_level) {
case RXRPC_SECURITY_PLAIN:
- break;
case RXRPC_SECURITY_AUTH:
- conn->size_align = 8;
- conn->security_size = sizeof(struct rxkad_level1_hdr);
- break;
case RXRPC_SECURITY_ENCRYPT:
- conn->size_align = 8;
- conn->security_size = sizeof(struct rxkad_level2_hdr);
break;
default:
ret = -EKEYREJECTED;
@@ -147,6 +142,40 @@ static int rxkad_init_connection_security(struct rxrpc_connection *conn,
return ret;
}
+/*
+ * Work out how much data we can put in a packet.
+ */
+static int rxkad_how_much_data(struct rxrpc_call *call, size_t remain,
+ size_t *_buf_size, size_t *_data_size, size_t *_offset)
+{
+ size_t shdr, buf_size, chunk;
+
+ switch (call->conn->params.security_level) {
+ default:
+ buf_size = chunk = min_t(size_t, remain, RXRPC_JUMBO_DATALEN);
+ shdr = 0;
+ goto out;
+ case RXRPC_SECURITY_AUTH:
+ shdr = sizeof(struct rxkad_level1_hdr);
+ break;
+ case RXRPC_SECURITY_ENCRYPT:
+ shdr = sizeof(struct rxkad_level2_hdr);
+ break;
+ }
+
+ buf_size = round_down(RXRPC_JUMBO_DATALEN, RXKAD_ALIGN);
+
+ chunk = buf_size - shdr;
+ if (remain < chunk)
+ buf_size = round_up(shdr + remain, RXKAD_ALIGN);
+
+out:
+ *_buf_size = buf_size;
+ *_data_size = chunk;
+ *_offset = shdr;
+ return 0;
+}
+
/*
* prime the encryption state with the invariant parts of a connection's
* description
@@ -237,6 +266,7 @@ static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
struct rxkad_level1_hdr hdr;
struct rxrpc_crypt iv;
struct scatterlist sg;
+ size_t pad;
u16 check;
_enter("");
@@ -247,6 +277,12 @@ static int rxkad_secure_packet_auth(const struct rxrpc_call *call,
hdr.data_size = htonl(data_size);
memcpy(skb->head, &hdr, sizeof(hdr));
+ pad = sizeof(struct rxkad_level1_hdr) + data_size;
+ pad = RXKAD_ALIGN - pad;
+ pad &= RXKAD_ALIGN - 1;
+ if (pad)
+ skb_put_zero(skb, pad);
+
/* start the encryption afresh */
memset(&iv, 0, sizeof(iv));
@@ -275,6 +311,7 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
struct rxrpc_crypt iv;
struct scatterlist sg[16];
unsigned int len;
+ size_t pad;
u16 check;
int err;
@@ -288,6 +325,12 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
rxkhdr.checksum = 0;
memcpy(skb->head, &rxkhdr, sizeof(rxkhdr));
+ pad = sizeof(struct rxkad_level2_hdr) + data_size;
+ pad = RXKAD_ALIGN - pad;
+ pad &= RXKAD_ALIGN - 1;
+ if (pad)
+ skb_put_zero(skb, pad);
+
/* encrypt from the session key */
token = call->conn->params.key->payload.data[0];
memcpy(&iv, token->kad->session_key, sizeof(iv));
@@ -303,8 +346,7 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
if (skb_shinfo(skb)->nr_frags > 16)
goto out;
- len = data_size + call->conn->size_align - 1;
- len &= ~(call->conn->size_align - 1);
+ len = round_up(data_size, RXKAD_ALIGN);
sg_init_table(sg, ARRAY_SIZE(sg));
err = skb_to_sgvec(skb, sg, 8, len);
@@ -1353,6 +1395,7 @@ const struct rxrpc_security rxkad = {
.free_preparse_server_key = rxkad_free_preparse_server_key,
.destroy_server_key = rxkad_destroy_server_key,
.init_connection_security = rxkad_init_connection_security,
+ .how_much_data = rxkad_how_much_data,
.secure_packet = rxkad_secure_packet,
.verify_packet = rxkad_verify_packet,
.free_call_crypto = rxkad_free_call_crypto,
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 367654a558c2..af8ad6c30b9f 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -327,7 +327,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
rxrpc_send_ack_packet(call, false, NULL);
if (!skb) {
- size_t size, chunk, max, space;
+ size_t remain, bufsize, chunk, offset;
_debug("alloc");
@@ -342,24 +342,21 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
goto maybe_error;
}
- max = RXRPC_JUMBO_DATALEN;
- max -= call->conn->security_size;
- max &= ~(call->conn->size_align - 1UL);
-
- chunk = max;
- if (chunk > msg_data_left(msg) && !more)
- chunk = msg_data_left(msg);
-
- space = chunk + call->conn->size_align;
- space &= ~(call->conn->size_align - 1UL);
-
- size = space + call->conn->security_size;
+ /* Work out the maximum size of a packet. Assume that
+ * the security header is going to be in the padded
+ * region (enc blocksize), but the trailer is not.
+ */
+ remain = more ? INT_MAX : msg_data_left(msg);
+ ret = call->conn->security->how_much_data(call, remain,
+ &bufsize, &chunk, &offset);
+ if (ret < 0)
+ goto maybe_error;
- _debug("SIZE: %zu/%zu/%zu", chunk, space, size);
+ _debug("SIZE: %zu/%zu @%zu", chunk, bufsize, offset);
/* create a buffer that we can retain until it's ACK'd */
skb = sock_alloc_send_skb(
- sk, size, msg->msg_flags & MSG_DONTWAIT, &ret);
+ sk, bufsize, msg->msg_flags & MSG_DONTWAIT, &ret);
if (!skb)
goto maybe_error;
@@ -371,8 +368,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
ASSERTCMP(skb->mark, ==, 0);
- _debug("HS: %u", call->conn->security_size);
- __skb_put(skb, call->conn->security_size);
+ __skb_put(skb, offset);
sp->remain = chunk;
if (sp->remain > skb_tailroom(skb))
@@ -421,17 +417,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
(msg_data_left(msg) == 0 && !more)) {
struct rxrpc_connection *conn = call->conn;
uint32_t seq;
- size_t pad;
-
- /* pad out if we're using security */
- if (conn->security_ix) {
- pad = conn->security_size + skb->mark;
- pad = conn->size_align - pad;
- pad &= conn->size_align - 1;
- _debug("pad %zu", pad);
- if (pad)
- skb_put_zero(skb, pad);
- }
seq = call->tx_top + 1;
^ permalink raw reply related
* Re: [PATCH net-next v2 1/2] lockdep: Introduce in_softirq lockdep assert
From: Jakub Kicinski @ 2020-11-23 20:12 UTC (permalink / raw)
To: Peter Zijlstra, Yunsheng Lin
Cc: mingo, will, viro, kyk.segfault, davem, linmiaohe,
martin.varghese, pabeni, pshelar, fw, gnault, steffen.klassert,
vladimir.oltean, edumazet, saeed, netdev, linux-kernel, linuxarm,
Thomas Gleixner
In-Reply-To: <20201123142725.GQ3021@hirez.programming.kicks-ass.net>
On Mon, 23 Nov 2020 15:27:25 +0100 Peter Zijlstra wrote:
> On Sat, Nov 21, 2020 at 11:06:15AM +0800, Yunsheng Lin wrote:
> > The current semantic for napi_consume_skb() is that caller need
> > to provide non-zero budget when calling from NAPI context, and
> > breaking this semantic will cause hard to debug problem, because
> > _kfree_skb_defer() need to run in atomic context in order to push
> > the skb to the particular cpu' napi_alloc_cache atomically.
> >
> > So add the lockdep_assert_in_softirq() to assert when the running
> > context is not in_softirq, in_softirq means softirq is serving or
> > BH is disabled. Because the softirq context can be interrupted by
> > hard IRQ or NMI context, so lockdep_assert_in_softirq() need to
> > assert about hard IRQ or NMI context too.
> Due to in_softirq() having a deprication notice (due to it being
> awefully ambiguous), could we have a nice big comment here that explains
> in detail understandable to !network people (me) why this is actually
> correct?
>
> I'm not opposed to the thing, if that his what you need, it's fine, but
> please put on a comment that explains that in_softirq() is ambiguous and
> when you really do need it anyway.
One liner would be:
* Acceptable for protecting per-CPU resources accessed from BH
We can add:
* Much like in_softirq() - semantics are ambiguous, use carefully. *
IIUC we basically want to protect the nc array and counter here:
static inline void _kfree_skb_defer(struct sk_buff *skb)
{
struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
/* drop skb->head and call any destructors for packet */
skb_release_all(skb);
/* record skb to CPU local list */
nc->skb_cache[nc->skb_count++] = skb;
#ifdef CONFIG_SLUB
/* SLUB writes into objects when freeing */
prefetchw(skb);
#endif
/* flush skb_cache if it is filled */
if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) {
kmem_cache_free_bulk(skbuff_head_cache, NAPI_SKB_CACHE_SIZE,
nc->skb_cache);
nc->skb_count = 0;
}
}
> > +#define lockdep_assert_in_softirq() \
> > +do { \
> > + WARN_ON_ONCE(__lockdep_enabled && \
> > + (!in_softirq() || in_irq() || in_nmi())); \
> > +} while (0)
^ permalink raw reply
* Re: [PATCH v2] net: mvpp2: divide fifo for dts-active ports only
From: Russell King - ARM Linux admin @ 2020-11-23 20:17 UTC (permalink / raw)
To: stefanc
Cc: netdev, thomas.petazzoni, davem, nadavh, ymarkman, linux-kernel,
kuba, mw, andrew
In-Reply-To: <1606154073-28267-1-git-send-email-stefanc@marvell.com>
On Mon, Nov 23, 2020 at 07:54:33PM +0200, stefanc@marvell.com wrote:
> From: Stefan Chulski <stefanc@marvell.com>
>
> Tx/Rx FIFO is a HW resource limited by total size, but shared
> by all ports of same CP110 and impacting port-performance.
> Do not divide the FIFO for ports which are not enabled in DTS,
> so active ports could have more FIFO.
> No change in FIFO allocation if all 3 ports on the communication
> processor enabled in DTS.
>
> The active port mapping should be done in probe before FIFO-init.
>
> Signed-off-by: Stefan Chulski <stefanc@marvell.com>
Thanks.
Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk>
One thing I didn't point out is that netdev would like patch submissions
to indicate which tree they are targetting. Are you intending this for
net or net-next?
[PATCH net vX] ...
or
[PATCH net-next vX] ...
in the subject line please.
> ---
> drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 23 +++--
> drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 129 +++++++++++++++++-------
> 2 files changed, 108 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> index 8347758..6bd7e40 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> @@ -695,6 +695,9 @@
> /* Maximum number of supported ports */
> #define MVPP2_MAX_PORTS 4
>
> +/* Loopback port index */
> +#define MVPP2_LOOPBACK_PORT_INDEX 3
> +
> /* Maximum number of TXQs used by single port */
> #define MVPP2_MAX_TXQ 8
>
> @@ -729,22 +732,21 @@
> #define MVPP2_TX_DESC_ALIGN (MVPP2_DESC_ALIGNED_SIZE - 1)
>
> /* RX FIFO constants */
> +#define MVPP2_RX_FIFO_PORT_DATA_SIZE_44KB 0xb000
> #define MVPP2_RX_FIFO_PORT_DATA_SIZE_32KB 0x8000
> #define MVPP2_RX_FIFO_PORT_DATA_SIZE_8KB 0x2000
> #define MVPP2_RX_FIFO_PORT_DATA_SIZE_4KB 0x1000
> -#define MVPP2_RX_FIFO_PORT_ATTR_SIZE_32KB 0x200
> -#define MVPP2_RX_FIFO_PORT_ATTR_SIZE_8KB 0x80
> +#define MVPP2_RX_FIFO_PORT_ATTR_SIZE(data_size) ((data_size) >> 6)
> #define MVPP2_RX_FIFO_PORT_ATTR_SIZE_4KB 0x40
> #define MVPP2_RX_FIFO_PORT_MIN_PKT 0x80
>
> /* TX FIFO constants */
> -#define MVPP22_TX_FIFO_DATA_SIZE_10KB 0xa
> -#define MVPP22_TX_FIFO_DATA_SIZE_3KB 0x3
> -#define MVPP2_TX_FIFO_THRESHOLD_MIN 256
> -#define MVPP2_TX_FIFO_THRESHOLD_10KB \
> - (MVPP22_TX_FIFO_DATA_SIZE_10KB * 1024 - MVPP2_TX_FIFO_THRESHOLD_MIN)
> -#define MVPP2_TX_FIFO_THRESHOLD_3KB \
> - (MVPP22_TX_FIFO_DATA_SIZE_3KB * 1024 - MVPP2_TX_FIFO_THRESHOLD_MIN)
> +#define MVPP22_TX_FIFO_DATA_SIZE_16KB 16
> +#define MVPP22_TX_FIFO_DATA_SIZE_10KB 10
> +#define MVPP22_TX_FIFO_DATA_SIZE_3KB 3
> +#define MVPP2_TX_FIFO_THRESHOLD_MIN 256 /* Bytes */
> +#define MVPP2_TX_FIFO_THRESHOLD(kb) \
> + ((kb) * 1024 - MVPP2_TX_FIFO_THRESHOLD_MIN)
>
> /* RX buffer constants */
> #define MVPP2_SKB_SHINFO_SIZE \
> @@ -946,6 +948,9 @@ struct mvpp2 {
> /* List of pointers to port structures */
> int port_count;
> struct mvpp2_port *port_list[MVPP2_MAX_PORTS];
> + /* Map of enabled ports */
> + unsigned long port_map;
> +
> struct mvpp2_tai *tai;
>
> /* Number of Tx threads used */
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index f6616c8..08c237a 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -6601,32 +6601,56 @@ static void mvpp2_rx_fifo_init(struct mvpp2 *priv)
> mvpp2_write(priv, MVPP2_RX_FIFO_INIT_REG, 0x1);
> }
>
> -static void mvpp22_rx_fifo_init(struct mvpp2 *priv)
> +static void mvpp22_rx_fifo_set_hw(struct mvpp2 *priv, int port, int data_size)
> {
> - int port;
> + int attr_size = MVPP2_RX_FIFO_PORT_ATTR_SIZE(data_size);
>
> - /* The FIFO size parameters are set depending on the maximum speed a
> - * given port can handle:
> - * - Port 0: 10Gbps
> - * - Port 1: 2.5Gbps
> - * - Ports 2 and 3: 1Gbps
> - */
> + mvpp2_write(priv, MVPP2_RX_DATA_FIFO_SIZE_REG(port), data_size);
> + mvpp2_write(priv, MVPP2_RX_ATTR_FIFO_SIZE_REG(port), attr_size);
> +}
>
> - mvpp2_write(priv, MVPP2_RX_DATA_FIFO_SIZE_REG(0),
> - MVPP2_RX_FIFO_PORT_DATA_SIZE_32KB);
> - mvpp2_write(priv, MVPP2_RX_ATTR_FIFO_SIZE_REG(0),
> - MVPP2_RX_FIFO_PORT_ATTR_SIZE_32KB);
> +/* Initialize TX FIFO's: the total FIFO size is 48kB on PPv2.2.
> + * 4kB fixed space must be assigned for the loopback port.
> + * Redistribute remaining avialable 44kB space among all active ports.
> + * Guarantee minimum 32kB for 10G port and 8kB for port 1, capable of 2.5G
> + * SGMII link.
> + */
> +static void mvpp22_rx_fifo_init(struct mvpp2 *priv)
> +{
> + int remaining_ports_count;
> + unsigned long port_map;
> + int size_remainder;
> + int port, size;
> +
> + /* The loopback requires fixed 4kB of the FIFO space assignment. */
> + mvpp22_rx_fifo_set_hw(priv, MVPP2_LOOPBACK_PORT_INDEX,
> + MVPP2_RX_FIFO_PORT_DATA_SIZE_4KB);
> + port_map = priv->port_map & ~BIT(MVPP2_LOOPBACK_PORT_INDEX);
> +
> + /* Set RX FIFO size to 0 for inactive ports. */
> + for_each_clear_bit(port, &port_map, MVPP2_LOOPBACK_PORT_INDEX)
> + mvpp22_rx_fifo_set_hw(priv, port, 0);
> +
> + /* Assign remaining RX FIFO space among all active ports. */
> + size_remainder = MVPP2_RX_FIFO_PORT_DATA_SIZE_44KB;
> + remaining_ports_count = hweight_long(port_map);
> +
> + for_each_set_bit(port, &port_map, MVPP2_LOOPBACK_PORT_INDEX) {
> + if (remaining_ports_count == 1)
> + size = size_remainder;
> + else if (port == 0)
> + size = max(size_remainder / remaining_ports_count,
> + MVPP2_RX_FIFO_PORT_DATA_SIZE_32KB);
> + else if (port == 1)
> + size = max(size_remainder / remaining_ports_count,
> + MVPP2_RX_FIFO_PORT_DATA_SIZE_8KB);
> + else
> + size = size_remainder / remaining_ports_count;
>
> - mvpp2_write(priv, MVPP2_RX_DATA_FIFO_SIZE_REG(1),
> - MVPP2_RX_FIFO_PORT_DATA_SIZE_8KB);
> - mvpp2_write(priv, MVPP2_RX_ATTR_FIFO_SIZE_REG(1),
> - MVPP2_RX_FIFO_PORT_ATTR_SIZE_8KB);
> + size_remainder -= size;
> + remaining_ports_count--;
>
> - for (port = 2; port < MVPP2_MAX_PORTS; port++) {
> - mvpp2_write(priv, MVPP2_RX_DATA_FIFO_SIZE_REG(port),
> - MVPP2_RX_FIFO_PORT_DATA_SIZE_4KB);
> - mvpp2_write(priv, MVPP2_RX_ATTR_FIFO_SIZE_REG(port),
> - MVPP2_RX_FIFO_PORT_ATTR_SIZE_4KB);
> + mvpp22_rx_fifo_set_hw(priv, port, size);
> }
>
> mvpp2_write(priv, MVPP2_RX_MIN_PKT_SIZE_REG,
> @@ -6634,24 +6658,53 @@ static void mvpp22_rx_fifo_init(struct mvpp2 *priv)
> mvpp2_write(priv, MVPP2_RX_FIFO_INIT_REG, 0x1);
> }
>
> -/* Initialize Tx FIFO's: the total FIFO size is 19kB on PPv2.2 and 10G
> - * interfaces must have a Tx FIFO size of 10kB. As only port 0 can do 10G,
> - * configure its Tx FIFO size to 10kB and the others ports Tx FIFO size to 3kB.
> +static void mvpp22_tx_fifo_set_hw(struct mvpp2 *priv, int port, int size)
> +{
> + int threshold = MVPP2_TX_FIFO_THRESHOLD(size);
> +
> + mvpp2_write(priv, MVPP22_TX_FIFO_SIZE_REG(port), size);
> + mvpp2_write(priv, MVPP22_TX_FIFO_THRESH_REG(port), threshold);
> +}
> +
> +/* Initialize TX FIFO's: the total FIFO size is 19kB on PPv2.2.
> + * 3kB fixed space must be assigned for the loopback port.
> + * Redistribute remaining avialable 16kB space among all active ports.
> + * The 10G interface should use 10kB (which is maximum possible size
> + * per single port).
> */
> static void mvpp22_tx_fifo_init(struct mvpp2 *priv)
> {
> - int port, size, thrs;
> -
> - for (port = 0; port < MVPP2_MAX_PORTS; port++) {
> - if (port == 0) {
> + int remaining_ports_count;
> + unsigned long port_map;
> + int size_remainder;
> + int port, size;
> +
> + /* The loopback requires fixed 3kB of the FIFO space assignment. */
> + mvpp22_tx_fifo_set_hw(priv, MVPP2_LOOPBACK_PORT_INDEX,
> + MVPP22_TX_FIFO_DATA_SIZE_3KB);
> + port_map = priv->port_map & ~BIT(MVPP2_LOOPBACK_PORT_INDEX);
> +
> + /* Set TX FIFO size to 0 for inactive ports. */
> + for_each_clear_bit(port, &port_map, MVPP2_LOOPBACK_PORT_INDEX)
> + mvpp22_tx_fifo_set_hw(priv, port, 0);
> +
> + /* Assign remaining TX FIFO space among all active ports. */
> + size_remainder = MVPP22_TX_FIFO_DATA_SIZE_16KB;
> + remaining_ports_count = hweight_long(port_map);
> +
> + for_each_set_bit(port, &port_map, MVPP2_LOOPBACK_PORT_INDEX) {
> + if (remaining_ports_count == 1)
> + size = min(size_remainder,
> + MVPP22_TX_FIFO_DATA_SIZE_10KB);
> + else if (port == 0)
> size = MVPP22_TX_FIFO_DATA_SIZE_10KB;
> - thrs = MVPP2_TX_FIFO_THRESHOLD_10KB;
> - } else {
> - size = MVPP22_TX_FIFO_DATA_SIZE_3KB;
> - thrs = MVPP2_TX_FIFO_THRESHOLD_3KB;
> - }
> - mvpp2_write(priv, MVPP22_TX_FIFO_SIZE_REG(port), size);
> - mvpp2_write(priv, MVPP22_TX_FIFO_THRESH_REG(port), thrs);
> + else
> + size = size_remainder / remaining_ports_count;
> +
> + size_remainder -= size;
> + remaining_ports_count--;
> +
> + mvpp22_tx_fifo_set_hw(priv, port, size);
> }
> }
>
> @@ -6952,6 +7005,12 @@ static int mvpp2_probe(struct platform_device *pdev)
> goto err_axi_clk;
> }
>
> + /* Map DTS-active ports. Should be done before FIFO mvpp2_init */
> + fwnode_for_each_available_child_node(fwnode, port_fwnode) {
> + if (!fwnode_property_read_u32(port_fwnode, "port-id", &i))
> + priv->port_map |= BIT(i);
> + }
> +
> /* Initialize network controller */
> err = mvpp2_init(pdev, priv);
> if (err < 0) {
> --
> 1.9.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
From: min.li.xe @ 2020-11-23 20:20 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
Feed kstrtou8 with NULL terminated string.
Changes since v1:
-Use strscpy instead of strncpy for safety.
Signed-off-by: Min Li <min.li.xe@renesas.com>
---
drivers/ptp/ptp_clockmatrix.c | 60 ++++++++++++++++++++++++++++++-------------
tools/bpf/example | 12 +++++++++
tools/bpf/novlan | 7 +++++
3 files changed, 61 insertions(+), 18 deletions(-)
create mode 100644 tools/bpf/example
create mode 100644 tools/bpf/novlan
diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index e020faf..d4e434b 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -103,42 +103,66 @@ static int timespec_to_char_array(struct timespec64 const *ts,
return 0;
}
-static int idtcm_strverscmp(const char *ver1, const char *ver2)
+static int idtcm_strverscmp(const char *version1, const char *version2)
{
u8 num1;
u8 num2;
int result = 0;
+ char ver1[16];
+ char ver2[16];
+ char *cur1;
+ char *cur2;
+ char *next1;
+ char *next2;
+
+ if (strscpy(ver1, version1, 16) < 0 ||
+ strscpy(ver2, version2, 16) < 0)
+ return -1;
+ cur1 = ver1;
+ cur2 = ver2;
/* loop through each level of the version string */
while (result == 0) {
+ next1 = strchr(cur1, '.');
+ next2 = strchr(cur2, '.');
+
+ /* kstrtou8 could fail for dot */
+ if (next1) {
+ *next1 = '\0';
+ next1++;
+ }
+
+ if (next2) {
+ *next2 = '\0';
+ next2++;
+ }
+
/* extract leading version numbers */
- if (kstrtou8(ver1, 10, &num1) < 0)
+ if (kstrtou8(cur1, 10, &num1) < 0)
return -1;
- if (kstrtou8(ver2, 10, &num2) < 0)
+ if (kstrtou8(cur2, 10, &num2) < 0)
return -1;
/* if numbers differ, then set the result */
if (num1 < num2)
+ return -1;
+ if (num1 > num2)
+ return 1;
+
+ /* if numbers are the same, go to next level */
+ if (!next1 && !next2)
+ break;
+ else if (!next1) {
result = -1;
- else if (num1 > num2)
+ } else if (!next2) {
result = 1;
- else {
- /* if numbers are the same, go to next level */
- ver1 = strchr(ver1, '.');
- ver2 = strchr(ver2, '.');
- if (!ver1 && !ver2)
- break;
- else if (!ver1)
- result = -1;
- else if (!ver2)
- result = 1;
- else {
- ver1++;
- ver2++;
- }
+ } else {
+ cur1 = next1;
+ cur2 = next2;
}
}
+
return result;
}
diff --git a/tools/bpf/example b/tools/bpf/example
new file mode 100644
index 0000000..a0ac81f
--- /dev/null
+++ b/tools/bpf/example
@@ -0,0 +1,12 @@
+ ldh [12]
+ jne #0x8100, nonvlan
+ ldh [16]
+ jne #0x88f7, bad
+ ldb [18]
+ ja test
+ nonvlan: jne #0x88f7, bad
+ ldb [14]
+ test: and #0x8
+ jeq #0, bad
+ good: ret #1500
+ bad: ret #0
diff --git a/tools/bpf/novlan b/tools/bpf/novlan
new file mode 100644
index 0000000..fe35288
--- /dev/null
+++ b/tools/bpf/novlan
@@ -0,0 +1,7 @@
+ ldh [12]
+ jne #0x88f7, bad
+ ldb [14]
+ and #0x8
+ jeq #0, bad
+ good: ret #1500
+ bad: ret #0
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 15/17] rxrpc: Organise connection security to use a union
From: Joe Perches @ 2020-11-23 20:25 UTC (permalink / raw)
To: David Howells, netdev; +Cc: linux-afs, linux-kernel
In-Reply-To: <160616230898.830164.7298470680786861832.stgit@warthog.procyon.org.uk>
On Mon, 2020-11-23 at 20:11 +0000, David Howells wrote:
> Organise the security information in the rxrpc_connection struct to use a
> union to allow for different data for different security classes.
Is there a known future purpose to this?
> diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
> @@ -448,9 +448,15 @@ struct rxrpc_connection {
> struct list_head proc_link; /* link in procfs list */
> struct list_head link; /* link in master connection list */
> struct sk_buff_head rx_queue; /* received conn-level packets */
> +
> const struct rxrpc_security *security; /* applied security module */
> - struct crypto_sync_skcipher *cipher; /* encryption handle */
> - struct rxrpc_crypt csum_iv; /* packet checksum base */
> + union {
> + struct {
> + struct crypto_sync_skcipher *cipher; /* encryption handle */
> + struct rxrpc_crypt csum_iv; /* packet checksum base */
> + u32 nonce; /* response re-use preventer */
> + } rxkad;
> + };
It seems no other follow-on patch in the series uses this nameless union.
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: James Bottomley @ 2020-11-23 20:37 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Kees Cook, Jakub Kicinski, Gustavo A. R. Silva, linux-kernel,
alsa-devel, amd-gfx, bridge, ceph-devel, cluster-devel, coreteam,
devel, dm-devel, drbd-dev, dri-devel, GR-everest-linux-l2,
GR-Linux-NIC-Dev, intel-gfx, intel-wired-lan, keyrings,
linux1394-devel, linux-acpi, linux-afs, Linux ARM, linux-arm-msm,
linux-atm-general, linux-block, linux-can, linux-cifs,
Linux Crypto Mailing List, linux-decnet-user,
Ext4 Developers List, linux-fbdev, linux-geode, linux-gpio,
linux-hams, linux-hwmon, linux-i3c, linux-ide, linux-iio,
linux-input, linux-integrity, linux-mediatek,
Linux Media Mailing List, linux-mmc, Linux-MM, linux-mtd,
linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi, linux-sctp,
linux-security-module, linux-stm32, linux-usb, linux-watchdog,
linux-wireless, Network Development, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda, Joe Perches
In-Reply-To: <CANiq72k5tpDoDPmJ0ZWc1DGqm+81Gi-uEENAtvEs9v3SZcx6_Q@mail.gmail.com>
On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote:
> On Mon, Nov 23, 2020 at 4:58 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > Well, I used git. It says that as of today in Linus' tree we have
> > 889 patches related to fall throughs and the first series went in
> > in october 2017 ... ignoring a couple of outliers back to February.
>
> I can see ~10k insertions over ~1k commits and 15 years that mention
> a fallthrough in the entire repo. That is including some commits
> (like the biggest one, 960 insertions) that have nothing to do with C
> fallthrough. A single kernel release has an order of magnitude more
> changes than this...
>
> But if we do the math, for an author, at even 1 minute per line
> change and assuming nothing can be automated at all, it would take 1
> month of work. For maintainers, a couple of trivial lines is noise
> compared to many other patches.
So you think a one line patch should take one minute to produce ... I
really don't think that's grounded in reality. I suppose a one line
patch only takes a minute to merge with b4 if no-one reviews or tests
it, but that's not really desirable.
> In fact, this discussion probably took more time than the time it
> would take to review the 200 lines. :-)
I'm framing the discussion in terms of the whole series of changes we
have done for fall through, both what's in the tree currently (889
patches) both in terms of the produce and the consumer. That's what I
used for my figures for cost.
> > We're also complaining about the inability to recruit maintainers:
> >
> > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/
> >
> > And burn out:
> >
> > http://antirez.com/news/129
>
> Accepting trivial and useful 1-line patches
Part of what I'm trying to measure is the "and useful" bit because
that's not a given.
> is not what makes a voluntary maintainer quit...
so the proverb "straw which broke the camel's back" uniquely doesn't
apply to maintainers
> Thankless work with demanding deadlines is.
That's another potential reason, but it doesn't may other reasons less
valid.
> > The whole crux of your argument seems to be maintainers' time isn't
> > important so we should accept all trivial patches
>
> I have not said that, at all. In fact, I am a voluntary one and I
> welcome patches like this. It takes very little effort on my side to
> review and it helps the kernel overall.
Well, you know, subsystems are very different in terms of the amount of
patches a maintainer has to process per release cycle of the kernel.
If a maintainer is close to capacity, additional patches, however
trivial, become a problem. If a maintainer has spare cycles, trivial
patches may look easy.
> Paid maintainers are the ones that can take care of big
> features/reviews.
>
> > What I'm actually trying to articulate is a way of measuring value
> > of the patch vs cost ... it has nothing really to do with who foots
> > the actual bill.
>
> I understand your point, but you were the one putting it in terms of
> a junior FTE.
No, I evaluated the producer side in terms of an FTE. What we're
mostly arguing about here is the consumer side: the maintainers and
people who have to rework their patch sets. I estimated that at 100h.
> In my view, 1 month-work (worst case) is very much worth
> removing a class of errors from a critical codebase.
>
> > One thesis I'm actually starting to formulate is that this
> > continual devaluing of maintainers is why we have so much
> > difficulty keeping and recruiting them.
>
> That may very well be true, but I don't feel anybody has devalued
> maintainers in this discussion.
You seem to be saying that because you find it easy to merge trivial
patches, everyone should. I'm reminded of a friend long ago who
thought being a Tees River Pilot was a sinecure because he could
navigate the Tees blindfold. What he forgot, of course, is that just
because it's easy with a trawler doesn't mean it's easy with an oil
tanker. In fact it takes longer to qualify as a Tees River Pilot than
it does to get a PhD.
James
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: dsa: add Arrow SpeedChips XRS700x driver
From: George McCollister @ 2020-11-23 20:43 UTC (permalink / raw)
To: Andrew Lunn
Cc: Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, netdev, open list:OPEN FIRMWARE AND...
In-Reply-To: <20201120232439.GA1949248@lunn.ch>
On Fri, Nov 20, 2020 at 5:24 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Hi George
>
> > > > +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> > > > + u8 state)
> > > > +{
> > > > + struct xrs700x *priv = ds->priv;
> > > > + unsigned int val;
> > > > +
> > > > + switch (state) {
> > > > + case BR_STATE_DISABLED:
> > > > + val = XRS_PORT_DISABLED;
> > > > + break;
> > > > + case BR_STATE_LISTENING:
> > > > + val = XRS_PORT_DISABLED;
> > > > + break;
> > >
> > > No listening state?
> >
> > No, just forwarding, learning and disabled. See:
> > https://www.flexibilis.com/downloads/xrs/SpeedChip_XRS7000_3000_User_Manual.pdf
> > page 82.
> >
> > >
> > > > + case BR_STATE_LEARNING:
> > > > + val = XRS_PORT_LEARNING;
> > > > + break;
> > > > + case BR_STATE_FORWARDING:
> > > > + val = XRS_PORT_FORWARDING;
> > > > + break;
> > > > + case BR_STATE_BLOCKING:
> > > > + val = XRS_PORT_DISABLED;
> > > > + break;
> > >
> > > Hum. What exactly does XRS_PORT_DISABLED mean? When blocking, it is
> > > expected you can still send/receive BPDUs.
> >
> > Datasheet says: "Disabled. Port neither learns MAC addresses nor forwards data."
>
> I think you need to do some testing here. Put the device into a loop
> with another switch, the bridge will block a port, and see if it still
> can send/receive BPDUs on the blocked port.
>
> If it cannot send/receive BPDUs, it might get into an oscillating
> state. They see each other via BPDUs, decide there is a loop, and
> block a port. The BPDUs stop, they think the loop has been broken and
> so unblock. They see each other via BPUS, decide there is a loop,...
Yeah, this is messed up. The switch doesn't seem to pass up BPDUs in
either disabled or learning mode, only forward mode.
Can I just replace .port_stp_state_set with .port_enable (setting
switch port to forward mode) and .port_disable (setting switch port to
disabled mode)? I don't see any other way around this. It looks like
rtl8366rb.c also has no .port_stp_state_set.
>
> > > > +static int xrs700x_i2c_reg_read(void *context, unsigned int reg,
> > > > + unsigned int *val)
> > > > +{
> > > > + int ret;
> > > > + unsigned char buf[4];
> > > > + struct device *dev = context;
> > > > + struct i2c_client *i2c = to_i2c_client(dev);
> > > > +
> > > > + buf[0] = reg >> 23 & 0xff;
> > > > + buf[1] = reg >> 15 & 0xff;
> > > > + buf[2] = reg >> 7 & 0xff;
> > > > + buf[3] = (reg & 0x7f) << 1;
> > > > +
> > > > + ret = i2c_master_send(i2c, buf, sizeof(buf));
> > >
> > > Are you allowed to perform transfers on stack buffers? I think any I2C
> > > bus driver using DMA is going to be unhappy.
> >
> > It should be fine. See the following file, there is a good write up about this:
> > See Documentation/i2c/dma-considerations.rst
>
> O.K, thanks for the pointer.
>
> > > > +static const struct of_device_id xrs700x_i2c_dt_ids[] = {
> > > > + { .compatible = "arrow,xrs7003" },
> > > > + { .compatible = "arrow,xrs7004" },
> > > > + {},
> > >
> > > Please validate that the compatible string actually matches the switch
> > > found. Otherwise we can get into all sorts of horrible backward
> > > compatibility issues.
> >
> > Okay. What kind of compatibility issues? Do you have a hypothetical
> > example? I guess I will just use of_device_is_compatible() to check.
>
> Since it currently does not matter, you can expect 50% of the boards
> to get it wrong. Sometime later, you find some difference between the
> two, you want to add additional optional properties dependent on the
> compatible string. But that is made hard, because 50% of the boards
> are broken, and the compatible string is now worthless.
>
> Either you need to verify the compatible from day one so it is not
> wrong, or you just use a single compatible "arrow,xrs700x", which
> cannot be wrong.
okay I'll make sure an error is returned if the detected switch does
not match compatible.
>
> Andrew
^ permalink raw reply
* Re: [PATCH net-next 15/17] rxrpc: Organise connection security to use a union
From: David Howells @ 2020-11-23 21:08 UTC (permalink / raw)
To: Joe Perches; +Cc: dhowells, netdev, linux-afs, linux-kernel
In-Reply-To: <d0d1edf746b8f50ca8897478a5e76a006e5d36ed.camel@perches.com>
Joe Perches <joe@perches.com> wrote:
> It seems no other follow-on patch in the series uses this nameless union.
There will be a follow on series. Either this:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-rxgk
or this:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=crypto-krb5
Depending on whether I pull the kerberos bits out into the crypto/ directory
so that it can be shared with sunrpc and maybe cifs. Discussions are ongoing
on that.
David
^ permalink raw reply
* Re: [PATCH mlx5-next 09/16] net/mlx5: Expose IP-in-IP TX and RX capability bits
From: Saeed Mahameed @ 2020-11-23 21:15 UTC (permalink / raw)
To: Aya Levin, Jakub Kicinski
Cc: Leon Romanovsky, netdev, linux-rdma, Moshe Shemesh
In-Reply-To: <7e783e5c-a4e1-bdc9-e5bb-73762f05ad19@nvidia.com>
On Sun, 2020-11-22 at 17:17 +0200, Aya Levin wrote:
>
> On 11/22/2020 1:58 AM, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 15:03:32 -0800 Saeed Mahameed wrote:
> > > From: Aya Levin <ayal@nvidia.com>
> > >
> > > Expose FW indication that it supports stateless offloads for IP
> > > over IP
> > > tunneled packets per direction. In some HW like ConnectX-4 IP-in-
> > > IP
> > > support is not symmetric, it supports steering on the inner
> > > header but
> > > it doesn't TX-Checksum and TSO. Add IP-in-IP capability per
> > > direction to
> > > cover this case as well.
> >
> > What's the use for the rx capability in Linux? We don't have an API
> > to
> > configure that AFAIK.
> >
> Correct, the rx capability bit is used by the driver to allow flow
> steering on the inner header.
Currently we use the global HW capability to enable flow steering on
inner header for RSS. in upcoming patch to net-next we will relax the
dependency on the global capability and will use the dedicated rx cap
instead.
^ permalink raw reply
* Re: [PATCH] net: mlx5e: fix fs_tcp.c build when IPV6 is not enabled
From: Saeed Mahameed @ 2020-11-23 21:19 UTC (permalink / raw)
To: Tariq Toukan, Randy Dunlap, netdev
Cc: kernel test robot, Boris Pismenny, Tariq Toukan, David S. Miller,
Jakub Kicinski
In-Reply-To: <bcb539df-901f-c5a5-697a-a022c1c3bfe5@gmail.com>
On Mon, 2020-11-23 at 12:08 +0200, Tariq Toukan wrote:
>
> On 11/22/2020 11:12 PM, Randy Dunlap wrote:
> > Fix build when CONFIG_IPV6 is not enabled by making a function
> > be built conditionally.
> >
> > Fixes these build errors and warnings:
> >
> > ../drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c: In
> > function 'accel_fs_tcp_set_ipv6_flow':
> > ../include/net/sock.h:380:34: error: 'struct sock_common' has no
> > member named 'skc_v6_daddr'; did you mean 'skc_daddr'?
> > 380 | #define sk_v6_daddr __sk_common.skc_v6_daddr
> > | ^~~~~~~~~~~~
> > ../drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c:55:14:
> > note: in expansion of macro 'sk_v6_daddr'
> > 55 | &sk->sk_v6_daddr, 16);
> > | ^~~~~~~~~~~
> > At top level:
> > ../drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c:47:13:
> > warning: 'accel_fs_tcp_set_ipv6_flow' defined but not used [-
> > Wunused-function]
> > 47 | static void accel_fs_tcp_set_ipv6_flow(struct
> > mlx5_flow_spec *spec, struct sock *sk)
> >
> > Fixes: 5229a96e59ec ("net/mlx5e: Accel, Expose flow steering API
> > for rules add/del")
> > Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Cc: Saeed Mahameed <saeedm@nvidia.com>
> > Cc: Boris Pismenny <borisp@nvidia.com>
> > Cc: Tariq Toukan <tariqt@mellanox.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > ---
>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
>
>
Applied to net-mlx5,
Thanks!
^ permalink raw reply
* Re: [PATCH iproute2-next 0/7] Convert a number of use-cases to parse_on_off(), print_on_off()
From: Nikolay Aleksandrov @ 2020-11-23 21:21 UTC (permalink / raw)
To: David Ahern, Petr Machata, netdev, stephen
In-Reply-To: <dcbc0c8b-feb8-634c-4eec-80afa0809c06@gmail.com>
On 17/11/2020 02:56, David Ahern wrote:
> On 11/14/20 3:53 PM, Petr Machata wrote:
>> Two helpers, parse_on_off() and print_on_off(), have been recently added to
>> lib/utils.c. Convert a number of instances of the same effective behavior
>> to calls to these helpers.
>>
>> Petr Machata (7):
>> bridge: link: Port over to parse_on_off()
>> bridge: link: Convert to use print_on_off()
>> ip: iplink: Convert to use parse_on_off()
>> ip: iplink_bridge_slave: Port over to parse_on_off()
>> ip: iplink_bridge_slave: Convert to use print_on_off()
>> ip: ipnetconf: Convert to use print_on_off()
>> ip: iptuntap: Convert to use print_on_off()
>>
>> bridge/link.c | 135 ++++++++++++++++++---------------------
>> ip/iplink.c | 47 +++++---------
>> ip/iplink_bridge_slave.c | 46 +++++--------
>> ip/ipnetconf.c | 28 ++++----
>> ip/iptuntap.c | 18 ++----
>> 5 files changed, 112 insertions(+), 162 deletions(-)
>>
>
> looks fine to me. Added Nik for a second set of eyes on the bridge changes.
>
It's much later, but the changes look good to me. Thanks!
Cheers,
Nik
^ permalink raw reply
* Re: [PATCH net-next] net: sched: alias action flags with TCA_ACT_ prefix
From: Jakub Kicinski @ 2020-11-23 21:22 UTC (permalink / raw)
To: Vlad Buslov; +Cc: netdev, davem, jhs, xiyou.wangcong, jiri
In-Reply-To: <20201121160902.808705-1-vlad@buslov.dev>
On Sat, 21 Nov 2020 18:09:02 +0200 Vlad Buslov wrote:
> Currently both filter and action flags use same "TCA_" prefix which makes
> them hard to distinguish to code and confusing for users. Create aliases
> for existing action flags constants with "TCA_ACT_" prefix.
>
> Signed-off-by: Vlad Buslov <vlad@buslov.dev>
Are we expecting to add both aliases for all new flags?
TCA_FLAG_TERSE_DUMP exists only in net-next, we could rename it, right?
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: dsa: add Arrow SpeedChips XRS700x driver
From: George McCollister @ 2020-11-23 21:22 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
netdev, open list:OPEN FIRMWARE AND...
In-Reply-To: <20201122233940.annzfjtaza2z4lub@skbuf>
On Sun, Nov 22, 2020 at 5:39 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi George,
>
> On Fri, Nov 20, 2020 at 12:16:26PM -0600, George McCollister wrote:
> > Add a driver with initial support for the Arrow SpeedChips XRS7000
> > series of gigabit Ethernet switch chips which are typically used in
> > critical networking applications.
> >
> > The switches have up to three RGMII ports and one RMII port.
> > Management to the switches can be performed over i2c or mdio.
> >
> > Support for advanced features such as PTP and
> > HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
> > may be added at a later date.
> >
> > Signed-off-by: George McCollister <george.mccollister@gmail.com>
> > ---
> > drivers/net/dsa/Kconfig | 26 ++
> > drivers/net/dsa/Makefile | 3 +
> > drivers/net/dsa/xrs700x.c | 529 +++++++++++++++++++++++++++++++++++++++++
> > drivers/net/dsa/xrs700x.h | 27 +++
> > drivers/net/dsa/xrs700x_i2c.c | 148 ++++++++++++
> > drivers/net/dsa/xrs700x_mdio.c | 160 +++++++++++++
> > drivers/net/dsa/xrs700x_reg.h | 205 ++++++++++++++++
>
> How much code do you plan to add to this driver? If it's going to
> include IEEE 1588 and HSR/PRP offloading, would it make sense to put its
> source code in a new folder now, to avoid doing that later?
I'll probably do that now. HSR/PRP offloading is planned but I foresee
that taking a while (hopefully there aren't too many major
roadblocks). IEEE 1588 is a maybe.
>
> > 7 files changed, 1098 insertions(+)
> > create mode 100644 drivers/net/dsa/xrs700x.c
> > create mode 100644 drivers/net/dsa/xrs700x.h
> > create mode 100644 drivers/net/dsa/xrs700x_i2c.c
> > create mode 100644 drivers/net/dsa/xrs700x_mdio.c
> > create mode 100644 drivers/net/dsa/xrs700x_reg.h
> >
> > diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> > index f6a0488589fc..e5ec3c602bcb 100644
> > --- a/drivers/net/dsa/Kconfig
> > +++ b/drivers/net/dsa/Kconfig
> > @@ -134,4 +134,30 @@ config NET_DSA_VITESSE_VSC73XX_PLATFORM
> > This enables support for the Vitesse VSC7385, VSC7388, VSC7395
> > and VSC7398 SparX integrated ethernet switches, connected over
> > a CPU-attached address bus and work in memory-mapped I/O mode.
> > +
> > +config NET_DSA_XRS700X
> > + tristate
> > + depends on NET_DSA
> > + select NET_DSA_TAG_XRS700X
> > + select REGMAP
> > + help
> > + This enables support for Arrow SpeedChips XRS7003/7004 gigabit
> > + Ethernet switches.
> > +
> > +config NET_DSA_XRS700X_I2C
> > + tristate "Arrow XRS7000X series switch in I2C mode"
> > + depends on NET_DSA && I2C
> > + select NET_DSA_XRS700X
> > + select REGMAP_I2C
> > + help
> > + Enable I2C support for Arrow SpeedChips XRS7003/7004 gigabit Ethernet
> > + switches.
> > +
> > +config NET_DSA_XRS700X_MDIO
> > + tristate "Arrow XRS7000X series switch in MDIO mode"
> > + depends on NET_DSA
> > + select NET_DSA_XRS700X
> > + help
> > + Enable MDIO support for Arrow SpeedChips XRS7003/7004 gigabit Ethernet
> > + switches.
> > endmenu
> > diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
> > index a84adb140a04..4528d6b57fc8 100644
> > --- a/drivers/net/dsa/Makefile
> > +++ b/drivers/net/dsa/Makefile
> > @@ -17,6 +17,9 @@ obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
> > obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
> > obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
> > obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
> > +obj-$(CONFIG_NET_DSA_XRS700X) += xrs700x.o
> > +obj-$(CONFIG_NET_DSA_XRS700X_I2C) += xrs700x_i2c.o
> > +obj-$(CONFIG_NET_DSA_XRS700X_MDIO) += xrs700x_mdio.o
> > obj-y += b53/
> > obj-y += hirschmann/
> > obj-y += microchip/
> > diff --git a/drivers/net/dsa/xrs700x.c b/drivers/net/dsa/xrs700x.c
> > new file mode 100644
> > index 000000000000..6cef3b534d5d
> > --- /dev/null
> > +++ b/drivers/net/dsa/xrs700x.c
> > @@ -0,0 +1,529 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 NovaTech LLC
> > + * George McCollister <george.mccollister@gmail.com>
> > + */
> > +
> > +#include <net/dsa.h>
> > +#include <linux/if_bridge.h>
> > +#include "xrs700x.h"
> > +#include "xrs700x_reg.h"
> > +
> > +#define XRS700X_MIB_INTERVAL msecs_to_jiffies(30000)
> > +
> > +#define XRS7003E_ID 0x100
> > +#define XRS7003F_ID 0x101
> > +#define XRS7004E_ID 0x200
> > +#define XRS7004F_ID 0x201
> > +
> > +struct xrs700x_model {
> > + unsigned int id;
> > + const char *name;
> > + size_t num_ports;
> > +};
> > +
> > +static const struct xrs700x_model xrs700x_models[] = {
> > + {XRS7003E_ID, "XRS7003E", 3},
> > + {XRS7003F_ID, "XRS7003F", 3},
> > + {XRS7004E_ID, "XRS7004E", 4},
> > + {XRS7004F_ID, "XRS7004F", 4},
> > +};
> > +
> > +struct xrs700x_mib {
> > + unsigned int offset;
> > + const char *name;
> > +};
> > +
> > +static const struct xrs700x_mib xrs700x_mibs[] = {
> > + {XRS_RX_GOOD_OCTETS_L(0), "rx_good_octets"},
> > + {XRS_RX_BAD_OCTETS_L(0), "rx_bad_octets"},
> > + {XRS_RX_UNICAST_L(0), "rx_unicast"},
> > + {XRS_RX_BROADCAST_L(0), "rx_broadcast"},
> > + {XRS_RX_MULTICAST_L(0), "rx_multicast"},
> > + {XRS_RX_UNDERSIZE_L(0), "rx_undersize"},
> > + {XRS_RX_FRAGMENTS_L(0), "rx_fragments"},
> > + {XRS_RX_OVERSIZE_L(0), "rx_oversize"},
> > + {XRS_RX_JABBER_L(0), "rx_jabber"},
> > + {XRS_RX_ERR_L(0), "rx_err"},
> > + {XRS_RX_CRC_L(0), "rx_crc"},
> > + {XRS_RX_64_L(0), "rx_64"},
> > + {XRS_RX_65_127_L(0), "rx_65_127"},
> > + {XRS_RX_128_255_L(0), "rx_128_255"},
> > + {XRS_RX_256_511_L(0), "rx_256_511"},
> > + {XRS_RX_512_1023_L(0), "rx_512_1023"},
> > + {XRS_RX_1024_1536_L(0), "rx_1024_1536"},
>
> Uh-oh, Jakub might not like these RMON counters being exposed to
> ethtool. See:
> https://patchwork.kernel.org/project/netdevbpf/patch/20201115073533.1366-1-o.rempel@pengutronix.de/
Great. So none of the DSA drivers do it the way he wants? Are there
plans to switch the existing drivers to what is discussed in that
thread? Can that even be done or will it "break userspace"? If someone
has an example for me to follow I can change this but this still seems
a bit up in the air.
>
> > + {XRS_RX_HSR_PRP_L(0), "rx_hsr_prp"},
> > + {XRS_RX_WRONGLAN_L(0), "rx_wronglan"},
> > + {XRS_RX_DUPLICATE_L(0), "rx_duplicate"},
> > + {XRS_TX_OCTETS_L(0), "tx_octets"},
> > + {XRS_TX_UNICAST_L(0), "tx_unicast"},
> > + {XRS_TX_BROADCAST_L(0), "tx_broadcast"},
> > + {XRS_TX_MULTICAST_L(0), "tx_multicast"},
> > + {XRS_TX_HSR_PRP_L(0), "tx_hsr_prp"},
> > + {XRS_PRIQ_DROP_L(0), "priq_drop"},
> > + {XRS_EARLY_DROP_L(0), "early_drop"},
> > +};
> > +
> > +static void xrs700x_get_strings(struct dsa_switch *ds, int port,
> > + u32 stringset, uint8_t *data)
> > +{
> > + int i;
> > +
> > + if (stringset != ETH_SS_STATS)
> > + return;
> > +
> > + for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) {
> > + strlcpy(data, xrs700x_mibs[i].name, ETH_GSTRING_LEN);
> > + data += ETH_GSTRING_LEN;
> > + }
> > +}
> > +
> > +static int xrs700x_get_sset_count(struct dsa_switch *ds, int port, int sset)
> > +{
> > + if (sset != ETH_SS_STATS)
> > + return -EOPNOTSUPP;
> > +
> > + return ARRAY_SIZE(xrs700x_mibs);
> > +}
> > +
> > +static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
> > +{
> > + int i;
> > + struct xrs700x_port *p = &priv->ports[port];
> > +
> > + mutex_lock(&p->mib_mutex);
> > +
> > + /* Capture counter values */
> > + regmap_write(priv->regmap, XRS_CNT_CTRL(port), 1);
> > +
> > + for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) {
> > + unsigned int high = 0, low = 0, reg;
> > +
> > + reg = xrs700x_mibs[i].offset + XRS_PORT_OFFSET * port;
> > + regmap_read(priv->regmap, reg, &low);
> > + regmap_read(priv->regmap, reg + 2, &high);
> > +
> > + p->mib_data[i] += (high << 16) | low;
> > + }
> > +
> > + mutex_unlock(&p->mib_mutex);
> > +}
> > +
> > +static void xrs700x_mib_work(struct work_struct *work)
> > +{
> > + struct xrs700x *priv = container_of(work, struct xrs700x,
> > + mib_work.work);
> > + int i;
> > +
> > + for (i = 0; i < priv->ds->num_ports; i++)
> > + xrs700x_read_port_counters(priv, i);
> > +
> > + schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL);
> > +}
> > +
> > +static void xrs700x_get_ethtool_stats(struct dsa_switch *ds, int port,
> > + uint64_t *data)
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + struct xrs700x_port *p = &priv->ports[port];
> > +
> > + xrs700x_read_port_counters(priv, port);
> > +
> > + mutex_lock(&p->mib_mutex);
> > + memcpy(data, p->mib_data, sizeof(*data) * ARRAY_SIZE(xrs700x_mibs));
> > + mutex_unlock(&p->mib_mutex);
> > +}
> > +
> > +static int xrs700x_setup_regmap_range(struct xrs700x *priv)
> > +{
> > + struct reg_field ps_forward = REG_FIELD_ID(XRS_PORT_STATE(0), 0, 1,
> > + priv->ds->num_ports,
> > + XRS_PORT_OFFSET);
>
> Oh, hey, another REG_FIELD_ID user!
I actually implemented this macro with a different name but someone
else upstreamed it before I could get to it.
>
> > + struct reg_field ps_management = REG_FIELD_ID(XRS_PORT_STATE(0), 2, 3,
> > + priv->ds->num_ports,
> > + XRS_PORT_OFFSET);
> > + struct reg_field ps_sel_speed = REG_FIELD_ID(XRS_PORT_STATE(0), 4, 9,
> > + priv->ds->num_ports,
> > + XRS_PORT_OFFSET);
> > + struct reg_field ps_cur_speed = REG_FIELD_ID(XRS_PORT_STATE(0), 10, 11,
> > + priv->ds->num_ports,
> > + XRS_PORT_OFFSET);
> > +
> > + priv->ps_forward = devm_regmap_field_alloc(priv->dev, priv->regmap,
> > + ps_forward);
> > + if (IS_ERR(priv->ps_forward))
> > + return PTR_ERR(priv->ps_forward);
> > +
> > + priv->ps_management = devm_regmap_field_alloc(priv->dev, priv->regmap,
> > + ps_management);
> > + if (IS_ERR(priv->ps_management))
> > + return PTR_ERR(priv->ps_management);
> > +
> > + priv->ps_sel_speed = devm_regmap_field_alloc(priv->dev, priv->regmap,
> > + ps_sel_speed);
> > + if (IS_ERR(priv->ps_sel_speed))
> > + return PTR_ERR(priv->ps_sel_speed);
> > +
> > + priv->ps_cur_speed = devm_regmap_field_alloc(priv->dev, priv->regmap,
> > + ps_cur_speed);
> > + if (IS_ERR(priv->ps_cur_speed))
> > + return PTR_ERR(priv->ps_cur_speed);
>
> Should you try to automate allocating these? You might get tired of
> adding and adding and adding to this function really quick. You might
> get some inspiration from ocelot_regfields_init() and that driver's use
> of regmap in general.
I'll look at it.
>
> > +
> > + return 0;
> > +}
> > +
> > +static enum dsa_tag_protocol xrs700x_get_tag_protocol(struct dsa_switch *ds,
> > + int port,
> > + enum dsa_tag_protocol m)
> > +{
> > + return DSA_TAG_PROTO_XRS700X;
> > +}
> > +
> > +static int xrs700x_reset(struct dsa_switch *ds)
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + int ret;
> > + unsigned int val;
> > +
> > + ret = regmap_write(priv->regmap, XRS_GENERAL, XRS_GENERAL_RESET);
> > + if (ret)
> > + goto error;
> > +
> > + ret = regmap_read_poll_timeout(priv->regmap, XRS_GENERAL,
> > + val, !(val & XRS_GENERAL_RESET),
> > + 10, 1000);
> > +error:
> > + if (ret) {
> > + dev_err_ratelimited(priv->dev, "error resetting switch: %d\n",
> > + ret);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> > + u8 state)
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + unsigned int val;
> > +
> > + switch (state) {
> > + case BR_STATE_DISABLED:
> > + val = XRS_PORT_DISABLED;
> > + break;
> > + case BR_STATE_LISTENING:
> > + val = XRS_PORT_DISABLED;
> > + break;
> > + case BR_STATE_LEARNING:
> > + val = XRS_PORT_LEARNING;
> > + break;
> > + case BR_STATE_FORWARDING:
> > + val = XRS_PORT_FORWARDING;
> > + break;
> > + case BR_STATE_BLOCKING:
> > + val = XRS_PORT_DISABLED;
>
> Why not just put BR_STATE_DISABLED and BR_STATE_BLOCKING one under the
> other?
This is all messed up anyway, see my last email.
>
> case BR_STATE_BLOCKING:
> case BR_STATE_DISABLED:
> val = XRS_PORT_DISABLED;
>
> > + break;
> > + default:
> > + dev_err(ds->dev, "invalid STP state: %d\n", state);
> > + return;
> > + }
> > +
> > + regmap_fields_write(priv->ps_forward, port, val);
> > +
> > + dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n",
> > + __func__, port, state, val);
> > +}
> > +
> > +static int xrs700x_port_setup(struct dsa_switch *ds, int port)
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + bool cpu_port = dsa_is_cpu_port(ds, port);
>
> Reverse Christmas tree notation please.
yeah, I already have updated this. It'll be in v2.
>
> > + unsigned int val;
>
> Ugh, you couldn't have initialized this with zero here? It looks ugly
> putting that in the for loop.
I never do that and disagree but if you want me to change it anyway,
it's not a battle worth fighting.
>
> > + int ret, i;
> > +
> > + xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED);
> > +
> > + /* Disable forwarding to non-CPU ports */
> > + for (val = 0, i = 0; i < ds->num_ports; i++) {
> > + if (!dsa_is_cpu_port(ds, i))
> > + val |= BIT(i);
> > + }
> > +
> > + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
> > + if (ret)
> > + return ret;
> > +
> > + val = cpu_port ? XRS_PORT_MODE_MANAGEMENT : XRS_PORT_MODE_NORMAL;
> > + ret = regmap_fields_write(priv->ps_management, port, val);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int xrs700x_setup(struct dsa_switch *ds)
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + int ret, i;
> > +
> > + ret = xrs700x_reset(ds);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < ds->num_ports; i++) {
> > + ret = xrs700x_port_setup(ds, i);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL);
> > +
> > + return 0;
> > +}
> > +
> > +static void xrs700x_phylink_validate(struct dsa_switch *ds, int port,
> > + unsigned long *supported,
> > + struct phylink_link_state *state)
> > +{
> > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > + switch (port) {
> > + case 0:
> > + break;
> > + case 1:
> > + case 2:
> > + case 3:
> > + phylink_set(mask, 1000baseT_Full);
> > + break;
> > + default:
> > + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> > + dev_err(ds->dev, "Unsupported port: %i\n", port);
> > + return;
> > + }
> > +
> > + phylink_set_port_modes(mask);
> > +
> > + /* The switch only supports full duplex. */
> > + phylink_set(mask, 10baseT_Full);
> > + phylink_set(mask, 100baseT_Full);
> > +
> > + bitmap_and(supported, supported, mask,
> > + __ETHTOOL_LINK_MODE_MASK_NBITS);
> > + bitmap_and(state->advertising, state->advertising, mask,
> > + __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +}
> > +
> > +static void xrs700x_phylink_mac_config(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + const struct phylink_link_state *state)
>
> As far as I understand phylink, you should be programming the link speed
> of the RGMII/RMII MAC from the .mac_link_up callback.
It's working as is but I'll try moving it to .mac_link_up.
>
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + unsigned int val;
> > +
> > + switch (state->speed) {
> > + case SPEED_1000:
> > + val = XRS_PORT_SPEED_1000;
> > + break;
> > + case SPEED_100:
> > + val = XRS_PORT_SPEED_100;
> > + break;
> > + case SPEED_10:
> > + val = XRS_PORT_SPEED_10;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + regmap_fields_write(priv->ps_sel_speed, port, val);
> > +
> > + dev_dbg_ratelimited(priv->dev, "%s: port: %d mode: %u speed: %u\n",
> > + __func__, port, mode, state->speed);
> > +}
> > +
> > +static int xrs700x_bridge_join(struct dsa_switch *ds, int port,
> > + struct net_device *bridge)
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + unsigned int i, ret, mask = 0;
> > +
> > + for (i = 0; i < ds->num_ports; i++) {
> > + if (dsa_to_port(ds, i)->bridge_dev == bridge ||
> > + dsa_is_cpu_port(ds, i))
> > + continue;
> > +
> > + mask |= BIT(i);
> > + }
> > +
> > + dev_dbg(priv->dev, "%s: port: %d mask: 0x%x\n", __func__,
> > + port, mask);
> > +
> > + for (i = 0; i < ds->num_ports; i++) {
> > + if (dsa_to_port(ds, i)->bridge_dev != bridge)
> > + continue;
> > +
> > + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void xrs700x_bridge_leave(struct dsa_switch *ds, int port,
> > + struct net_device *bridge)
>
> Don't be lazy, you can avoid copy-pasting the implementation for this
> one...
Okay I'll write a common function for join and leave to use.
>
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + unsigned int i, cpu_mask = 0, mask = 0;
> > +
> > + for (i = 0; i < ds->num_ports; i++) {
> > + if (dsa_is_cpu_port(ds, i))
> > + continue;
> > +
> > + cpu_mask |= BIT(i);
> > +
> > + if (dsa_to_port(ds, i)->bridge_dev == bridge)
> > + continue;
> > +
> > + mask |= BIT(i);
> > + }
> > +
> > + dev_dbg(priv->dev, "%s: port: %d mask: 0x%x\n", __func__,
> > + port, mask);
> > +
> > + for (i = 0; i < ds->num_ports; i++) {
> > + if (dsa_to_port(ds, i)->bridge_dev != bridge)
> > + continue;
> > +
> > + regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);
> > + }
> > +
> > + regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), cpu_mask);
> > +}
> > +
> > +static const struct dsa_switch_ops xrs700x_ops = {
> > + .get_tag_protocol = xrs700x_get_tag_protocol,
> > + .setup = xrs700x_setup,
> > + .port_stp_state_set = xrs700x_port_stp_state_set,
> > + .phylink_validate = xrs700x_phylink_validate,
> > + .phylink_mac_config = xrs700x_phylink_mac_config,
> > + .get_strings = xrs700x_get_strings,
> > + .get_sset_count = xrs700x_get_sset_count,
> > + .get_ethtool_stats = xrs700x_get_ethtool_stats,
> > + .port_bridge_join = xrs700x_bridge_join,
> > + .port_bridge_leave = xrs700x_bridge_leave,
> > +};
> > +
> > +static int xrs700x_detect(struct xrs700x *dev)
> > +{
> > + int i, ret;
> > + unsigned int id;
> > +
> > + ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id);
> > + if (ret) {
> > + dev_err(dev->dev, "error %d while reading switch id.\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(xrs700x_models); i++) {
> > + if (xrs700x_models[i].id == id) {
> > + dev->ds->num_ports = xrs700x_models[i].num_ports;
> > + dev_info(dev->dev, "%s detected.\n",
> > + xrs700x_models[i].name);
> > + return 0;
> > + }
> > + }
> > +
> > + dev_err(dev->dev, "unknown switch id 0x%x.\n", id);
> > +
> > + return -ENODEV;
> > +}
> > +
> > +struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv)
> > +{
> > + struct dsa_switch *ds;
> > + struct xrs700x *dev;
> > +
> > + ds = devm_kzalloc(base, sizeof(*ds), GFP_KERNEL);
> > + if (!ds)
> > + return NULL;
> > +
> > + ds->dev = base;
> > + ds->num_ports = DSA_MAX_PORTS;
>
> Why so many?
I've removed ds->num_ports = DSA_MAX_PORTS; for v2.
>
> > +
> > + dev = devm_kzalloc(base, sizeof(*dev), GFP_KERNEL);
> > + if (!dev)
> > + return NULL;
> > +
> > + INIT_DELAYED_WORK(&dev->mib_work, xrs700x_mib_work);
> > +
> > + ds->ops = &xrs700x_ops;
> > + ds->priv = dev;
> > + dev->dev = base;
> > +
> > + dev->ds = ds;
> > + dev->priv = priv;
> > +
> > + return dev;
> > +}
> > +EXPORT_SYMBOL(xrs700x_switch_alloc);
> > +
> > +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port)
> > +{
> > + struct xrs700x_port *p = &dev->ports[port];
> > + size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs);
> > +
> > + p->mib_data = devm_kzalloc(dev->dev, mib_size, GFP_KERNEL);
> > + if (!p->mib_data)
> > + return -ENOMEM;
> > +
> > + mutex_init(&p->mib_mutex);
> > +
> > + return 0;
> > +}
> > +
> > +int xrs700x_switch_register(struct xrs700x *dev)
> > +{
> > + int ret;
> > + int i;
> > +
> > + ret = xrs700x_detect(dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = xrs700x_setup_regmap_range(dev);
> > + if (ret)
> > + return ret;
> > +
> > + dev->ports = devm_kzalloc(dev->dev,
> > + sizeof(*dev->ports) * dev->ds->num_ports,
> > + GFP_KERNEL);
> > + if (!dev->ports)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < dev->ds->num_ports; i++) {
> > + ret = xrs700x_alloc_port_mib(dev, i);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = dsa_register_switch(dev->ds);
> > +
> > + if (ret)
> > + cancel_delayed_work_sync(&dev->mib_work);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(xrs700x_switch_register);
> > +
> > +void xrs700x_switch_remove(struct xrs700x *dev)
> > +{
> > + cancel_delayed_work_sync(&dev->mib_work);
> > +
> > + dsa_unregister_switch(dev->ds);
> > +}
> > +EXPORT_SYMBOL(xrs700x_switch_remove);
> > +
> > +MODULE_AUTHOR("George McCollister <george.mccollister@gmail.com>");
> > +MODULE_DESCRIPTION("Arrow SpeedChips XRS700x DSA driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/net/dsa/xrs700x.h b/drivers/net/dsa/xrs700x.h
> > new file mode 100644
> > index 000000000000..53acf4359477
> > --- /dev/null
> > +++ b/drivers/net/dsa/xrs700x.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <linux/device.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct xrs700x_port {
> > + struct mutex mib_mutex; /* protects mib_data */
> > + uint64_t *mib_data;
> > +};
> > +
> > +struct xrs700x {
> > + struct dsa_switch *ds;
> > + struct device *dev;
> > + void *priv;
> > + struct regmap *regmap;
> > + struct regmap_field *ps_forward;
> > + struct regmap_field *ps_management;
> > + struct regmap_field *ps_sel_speed;
> > + struct regmap_field *ps_cur_speed;
> > + struct delayed_work mib_work;
> > + struct xrs700x_port *ports;
> > +};
> > +
> > +struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv);
> > +int xrs700x_switch_register(struct xrs700x *dev);
> > +void xrs700x_switch_remove(struct xrs700x *dev);
> > diff --git a/drivers/net/dsa/xrs700x_i2c.c b/drivers/net/dsa/xrs700x_i2c.c
> > new file mode 100644
> > index 000000000000..30f6c5ce825b
> > --- /dev/null
> > +++ b/drivers/net/dsa/xrs700x_i2c.c
> > @@ -0,0 +1,148 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 NovaTech LLC
> > + * George McCollister <george.mccollister@gmail.com>
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include "xrs700x.h"
> > +#include "xrs700x_reg.h"
> > +
> > +static int xrs700x_i2c_reg_read(void *context, unsigned int reg,
> > + unsigned int *val)
> > +{
> > + int ret;
> > + unsigned char buf[4];
> > + struct device *dev = context;
> > + struct i2c_client *i2c = to_i2c_client(dev);
>
> Please sort variable declaration in the order of decreasing line length.
I have that done for v2. However in this case dev will still be before
i2c since the i2c line depends on it.
>
> > +
> > + buf[0] = reg >> 23 & 0xff;
> > + buf[1] = reg >> 15 & 0xff;
> > + buf[2] = reg >> 7 & 0xff;
> > + buf[3] = (reg & 0x7f) << 1;
> > +
> > + ret = i2c_master_send(i2c, buf, sizeof(buf));
> > + if (ret < 0) {
> > + dev_err(dev, "xrs i2c_master_send returned %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = i2c_master_recv(i2c, buf, 2);
> > + if (ret < 0) {
> > + dev_err(dev, "xrs i2c_master_recv returned %d\n", ret);
> > + return ret;
> > + }
> > +
> > + *val = buf[0] << 8 | buf[1];
> > +
> > + return 0;
> > +}
^ permalink raw reply
* Re: [PATCH net-next v2] compat: always include linux/compat.h from net/compat.h
From: patchwork-bot+netdevbpf @ 2020-11-23 21:40 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, hch, arnd
In-Reply-To: <20201121214844.1488283-1-kuba@kernel.org>
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Sat, 21 Nov 2020 13:48:44 -0800 you wrote:
> We're about to do reshuffling in networking headers and
> eliminate some implicit includes. This results in:
>
> In file included from ../net/ipv4/netfilter/arp_tables.c:26:
> include/net/compat.h:60:40: error: unknown type name ‘compat_uptr_t’; did you mean ‘compat_ptr_ioctl’?
> struct sockaddr __user **save_addr, compat_uptr_t *ptr,
> ^~~~~~~~~~~~~
> compat_ptr_ioctl
> include/net/compat.h:61:4: error: unknown type name ‘compat_size_t’; did you mean ‘compat_sigset_t’?
> compat_size_t *len);
> ^~~~~~~~~~~~~
> compat_sigset_t
>
> [...]
Here is the summary with links:
- [net-next,v2] compat: always include linux/compat.h from net/compat.h
https://git.kernel.org/netdev/net-next/c/fc0d3b24bdb7
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* [PATCH net v5] aquantia: Remove the build_skb path
From: Ramsay, Lincoln @ 2020-11-23 21:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Igor Russkikh, David S. Miller,
netdev@vger.kernel.org, Dmitry Bogdanov
In-Reply-To: <20201123084243.423b23a4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
From: Lincoln Ramsay <lincoln.ramsay@opengear.com>
When performing IPv6 forwarding, there is an expectation that SKBs
will have some headroom. When forwarding a packet from the aquantia
driver, this does not always happen, triggering a kernel warning.
aq_ring.c has this code (edited slightly for brevity):
if (buff->is_eop && buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
skb = build_skb(aq_buf_vaddr(&buff->rxdata), AQ_CFG_RX_FRAME_MAX);
} else {
skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
There is a significant difference between the SKB produced by these
2 code paths. When napi_alloc_skb creates an SKB, there is a certain
amount of headroom reserved. However, this is not done in the
build_skb codepath.
As the hardware buffer that build_skb is built around does not
handle the presence of the SKB header, this code path is being
removed and the napi_alloc_skb path will always be used. This code
path does have to copy the packet header into the SKB, but it adds
the packet data as a frag.
Fixes: 018423e90bee ("net: ethernet: aquantia: Add ring support code")
Signed-off-by: Lincoln Ramsay <lincoln.ramsay@opengear.com>
---
.../net/ethernet/aquantia/atlantic/aq_ring.c | 126 ++++++++----------
1 file changed, 52 insertions(+), 74 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 4f913658eea4..24122ccda614 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -413,85 +413,63 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
buff->rxdata.pg_off,
buff->len, DMA_FROM_DEVICE);
- /* for single fragment packets use build_skb() */
- if (buff->is_eop &&
- buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
- skb = build_skb(aq_buf_vaddr(&buff->rxdata),
+ skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
+ if (unlikely(!skb)) {
+ u64_stats_update_begin(&self->stats.rx.syncp);
+ self->stats.rx.skb_alloc_fails++;
+ u64_stats_update_end(&self->stats.rx.syncp);
+ err = -ENOMEM;
+ goto err_exit;
+ }
+ if (is_ptp_ring)
+ buff->len -=
+ aq_ptp_extract_ts(self->aq_nic, skb,
+ aq_buf_vaddr(&buff->rxdata),
+ buff->len);
+
+ hdr_len = buff->len;
+ if (hdr_len > AQ_CFG_RX_HDR_SIZE)
+ hdr_len = eth_get_headlen(skb->dev,
+ aq_buf_vaddr(&buff->rxdata),
+ AQ_CFG_RX_HDR_SIZE);
+
+ memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
+ ALIGN(hdr_len, sizeof(long)));
+
+ if (buff->len - hdr_len > 0) {
+ skb_add_rx_frag(skb, 0, buff->rxdata.page,
+ buff->rxdata.pg_off + hdr_len,
+ buff->len - hdr_len,
AQ_CFG_RX_FRAME_MAX);
- if (unlikely(!skb)) {
- u64_stats_update_begin(&self->stats.rx.syncp);
- self->stats.rx.skb_alloc_fails++;
- u64_stats_update_end(&self->stats.rx.syncp);
- err = -ENOMEM;
- goto err_exit;
- }
- if (is_ptp_ring)
- buff->len -=
- aq_ptp_extract_ts(self->aq_nic, skb,
- aq_buf_vaddr(&buff->rxdata),
- buff->len);
- skb_put(skb, buff->len);
page_ref_inc(buff->rxdata.page);
- } else {
- skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
- if (unlikely(!skb)) {
- u64_stats_update_begin(&self->stats.rx.syncp);
- self->stats.rx.skb_alloc_fails++;
- u64_stats_update_end(&self->stats.rx.syncp);
- err = -ENOMEM;
- goto err_exit;
- }
- if (is_ptp_ring)
- buff->len -=
- aq_ptp_extract_ts(self->aq_nic, skb,
- aq_buf_vaddr(&buff->rxdata),
- buff->len);
-
- hdr_len = buff->len;
- if (hdr_len > AQ_CFG_RX_HDR_SIZE)
- hdr_len = eth_get_headlen(skb->dev,
- aq_buf_vaddr(&buff->rxdata),
- AQ_CFG_RX_HDR_SIZE);
-
- memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
- ALIGN(hdr_len, sizeof(long)));
-
- if (buff->len - hdr_len > 0) {
- skb_add_rx_frag(skb, 0, buff->rxdata.page,
- buff->rxdata.pg_off + hdr_len,
- buff->len - hdr_len,
- AQ_CFG_RX_FRAME_MAX);
- page_ref_inc(buff->rxdata.page);
- }
+ }
- if (!buff->is_eop) {
- buff_ = buff;
- i = 1U;
- do {
- next_ = buff_->next,
- buff_ = &self->buff_ring[next_];
+ if (!buff->is_eop) {
+ buff_ = buff;
+ i = 1U;
+ do {
+ next_ = buff_->next;
+ buff_ = &self->buff_ring[next_];
- dma_sync_single_range_for_cpu(
- aq_nic_get_dev(self->aq_nic),
- buff_->rxdata.daddr,
- buff_->rxdata.pg_off,
- buff_->len,
- DMA_FROM_DEVICE);
- skb_add_rx_frag(skb, i++,
- buff_->rxdata.page,
- buff_->rxdata.pg_off,
- buff_->len,
- AQ_CFG_RX_FRAME_MAX);
- page_ref_inc(buff_->rxdata.page);
- buff_->is_cleaned = 1;
-
- buff->is_ip_cso &= buff_->is_ip_cso;
- buff->is_udp_cso &= buff_->is_udp_cso;
- buff->is_tcp_cso &= buff_->is_tcp_cso;
- buff->is_cso_err |= buff_->is_cso_err;
+ dma_sync_single_range_for_cpu(aq_nic_get_dev(self->aq_nic),
+ buff_->rxdata.daddr,
+ buff_->rxdata.pg_off,
+ buff_->len,
+ DMA_FROM_DEVICE);
+ skb_add_rx_frag(skb, i++,
+ buff_->rxdata.page,
+ buff_->rxdata.pg_off,
+ buff_->len,
+ AQ_CFG_RX_FRAME_MAX);
+ page_ref_inc(buff_->rxdata.page);
+ buff_->is_cleaned = 1;
- } while (!buff_->is_eop);
- }
+ buff->is_ip_cso &= buff_->is_ip_cso;
+ buff->is_udp_cso &= buff_->is_udp_cso;
+ buff->is_tcp_cso &= buff_->is_tcp_cso;
+ buff->is_cso_err |= buff_->is_cso_err;
+
+ } while (!buff_->is_eop);
}
if (buff->is_vlan)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net 02/15] ibmvnic: process HMC disable command
From: drt @ 2020-11-23 21:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Lijun Pan, netdev, sukadev, drt
In-Reply-To: <20201123114328.00c0b933@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On 2020-11-23 11:43, Jakub Kicinski wrote:
> On Sun, 22 Nov 2020 07:12:38 -0800 drt wrote:
>> On 2020-11-21 15:36, Jakub Kicinski wrote:
>> > On Fri, 20 Nov 2020 16:40:36 -0600 Lijun Pan wrote:
>> >> From: Dany Madden <drt@linux.ibm.com>
>> >>
>> >> Currently ibmvnic does not support the disable vnic command from the
>> >> Hardware Management Console. This patch enables ibmvnic to process
>> >> CRQ message 0x07, disable vnic adapter.
>> >
>> > What user-visible problem does this one solve?
>> This allows HMC to disconnect a Linux client from the network if the
>> vNIC adapter is misbehaving and/or sending malicious traffic. The
>> effect
>> is the same as when a sysadmin sets a link down (ibmvnic_close()) on
>> the
>> Linux client. This patch extends this ability to the HMC.
>
> Okay, sounds to me like net-next material, then.
>
> IIUC we don't need to fix this ASAP and backport to stable.
Yes, I will submit v2 net-next. Thank you.
^ 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