* [bpf PATCH v5 2/4] bpf: sockmap, fix smap_list_map_remove when psock is in many maps
From: John Fastabend @ 2018-06-30 13:17 UTC (permalink / raw)
To: ast, daniel, kafai; +Cc: netdev
In-Reply-To: <20180630131506.17344.7833.stgit@john-Precision-Tower-5810>
If a hashmap is free'd with open socks it removes the reference to
the hash entry from the psock. If that is the last reference to the
psock then it will also be free'd by the reference counting logic.
However the current logic that removes the hash reference from the
list of references is broken. In smap_list_remove() we first check
if the sockmap entry matches and then check if the hashmap entry
matches. But, the sockmap entry sill always match because its NULL in
this case which causes the first entry to be removed from the list.
If this is always the "right" entry (because the user adds/removes
entries in order) then everything is OK but otherwise a subsequent
bpf_tcp_close() may reference a free'd object.
To fix this create two list handlers one for sockmap and one for
sockhash.
Reported-by: syzbot+0ce137753c78f7b6acc1@syzkaller.appspotmail.com
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Acked-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/sockmap.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index d7fd17a..5adff4b 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1602,17 +1602,27 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
return ERR_PTR(err);
}
-static void smap_list_remove(struct smap_psock *psock,
- struct sock **entry,
- struct htab_elem *hash_link)
+static void smap_list_map_remove(struct smap_psock *psock,
+ struct sock **entry)
{
struct smap_psock_map_entry *e, *tmp;
list_for_each_entry_safe(e, tmp, &psock->maps, list) {
- if (e->entry == entry || e->hash_link == hash_link) {
+ if (e->entry == entry)
+ list_del(&e->list);
+ }
+}
+
+static void smap_list_hash_remove(struct smap_psock *psock,
+ struct htab_elem *hash_link)
+{
+ struct smap_psock_map_entry *e, *tmp;
+
+ list_for_each_entry_safe(e, tmp, &psock->maps, list) {
+ struct htab_elem *c = e->hash_link;
+
+ if (c == hash_link)
list_del(&e->list);
- break;
- }
}
}
@@ -1647,7 +1657,7 @@ static void sock_map_free(struct bpf_map *map)
* to be null and queued for garbage collection.
*/
if (likely(psock)) {
- smap_list_remove(psock, &stab->sock_map[i], NULL);
+ smap_list_map_remove(psock, &stab->sock_map[i]);
smap_release_sock(psock, sock);
}
write_unlock_bh(&sock->sk_callback_lock);
@@ -1706,7 +1716,7 @@ static int sock_map_delete_elem(struct bpf_map *map, void *key)
if (psock->bpf_parse)
smap_stop_sock(psock, sock);
- smap_list_remove(psock, &stab->sock_map[k], NULL);
+ smap_list_map_remove(psock, &stab->sock_map[k]);
smap_release_sock(psock, sock);
out:
write_unlock_bh(&sock->sk_callback_lock);
@@ -1908,7 +1918,7 @@ static int sock_map_ctx_update_elem(struct bpf_sock_ops_kern *skops,
struct smap_psock *opsock = smap_psock_sk(osock);
write_lock_bh(&osock->sk_callback_lock);
- smap_list_remove(opsock, &stab->sock_map[i], NULL);
+ smap_list_map_remove(opsock, &stab->sock_map[i]);
smap_release_sock(opsock, osock);
write_unlock_bh(&osock->sk_callback_lock);
}
@@ -2124,7 +2134,7 @@ static void sock_hash_free(struct bpf_map *map)
* (psock) to be null and queued for garbage collection.
*/
if (likely(psock)) {
- smap_list_remove(psock, NULL, l);
+ smap_list_hash_remove(psock, l);
smap_release_sock(psock, sock);
}
write_unlock_bh(&sock->sk_callback_lock);
@@ -2304,7 +2314,7 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
psock = smap_psock_sk(l_old->sk);
hlist_del_rcu(&l_old->hash_node);
- smap_list_remove(psock, NULL, l_old);
+ smap_list_hash_remove(psock, l_old);
smap_release_sock(psock, l_old->sk);
free_htab_elem(htab, l_old);
}
@@ -2372,7 +2382,7 @@ static int sock_hash_delete_elem(struct bpf_map *map, void *key)
* to be null and queued for garbage collection.
*/
if (likely(psock)) {
- smap_list_remove(psock, NULL, l);
+ smap_list_hash_remove(psock, l);
smap_release_sock(psock, sock);
}
write_unlock_bh(&sock->sk_callback_lock);
^ permalink raw reply related
* [bpf PATCH v5 3/4] bpf: sockhash fix omitted bucket lock in sock_close
From: John Fastabend @ 2018-06-30 13:17 UTC (permalink / raw)
To: ast, daniel, kafai; +Cc: netdev
In-Reply-To: <20180630131506.17344.7833.stgit@john-Precision-Tower-5810>
First the sk_callback_lock() was being used to protect both the
sock callback hooks and the psock->maps list. This got overly
convoluted after the addition of sockhash (in sockmap it made
some sense because masp and callbacks were tightly coupled) so
lets split out a specific lock for maps and only use the callback
lock for its intended purpose. This fixes a couple cases where
we missed using maps lock when it was in fact needed. Also this
makes it easier to follow the code because now we can put the
locking closer to the actual code its serializing.
Next, in sock_hash_delete_elem() the pattern was as follows,
sock_hash_delete_elem()
[...]
spin_lock(bucket_lock)
l = lookup_elem_raw()
if (l)
hlist_del_rcu()
write_lock(sk_callback_lock)
.... destroy psock ...
write_unlock(sk_callback_lock)
spin_unlock(bucket_lock)
The ordering is necessary because we only know the {p}sock after
dereferencing the hash table which we can't do unless we have the
bucket lock held. Once we have the bucket lock and the psock element
it is deleted from the hashmap to ensure any other path doing a lookup
will fail. Finally, the refcnt is decremented and if zero the psock
is destroyed.
In parallel with the above (or free'ing the map) a tcp close event
may trigger tcp_close(). Which at the moment omits the bucket lock
altogether (oops!) where the flow looks like this,
bpf_tcp_close()
[...]
write_lock(sk_callback_lock)
for each psock->maps // list of maps this sock is part of
hlist_del_rcu(ref_hash_node);
.... destroy psock ...
write_unlock(sk_callback_lock)
Obviously, and demonstrated by syzbot, this is broken because
we can have multiple threads deleting entries via hlist_del_rcu().
To fix this we might be tempted to wrap the hlist operation in a
bucket lock but that would create a lock inversion problem. In
summary to follow locking rules the psocks maps list needs the
sk_callback_lock (after this patch maps_lock) but we need the bucket
lock to do the hlist_del_rcu.
To resolve the lock inversion problem pop the head of the maps list
repeatedly and remove the reference until no more are left. If a
delete happens in parallel from the BPF API that is OK as well because
it will do a similar action, lookup the lock in the map/hash, delete
it from the map/hash, and dec the refcnt. We check for this case
before doing a destroy on the psock to ensure we don't have two
threads tearing down a psock. The new logic is as follows,
bpf_tcp_close()
e = psock_map_pop(psock->maps) // done with map lock
bucket_lock() // lock hash list bucket
l = lookup_elem_raw(head, hash, key, key_size);
if (l) {
//only get here if elmnt was not already removed
hlist_del_rcu()
... destroy psock...
}
bucket_unlock()
And finally for all the above to work add missing locking around map
operations per above. Then add RCU annotations and use
rcu_dereference/rcu_assign_pointer to manage values relying on RCU so
that the object is not free'd from sock_hash_free() while it is being
referenced in bpf_tcp_close().
Reported-by: syzbot+0ce137753c78f7b6acc1@syzkaller.appspotmail.com
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/sockmap.c | 145 +++++++++++++++++++++++++++++++++-----------------
1 file changed, 96 insertions(+), 49 deletions(-)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 5adff4b..60b6e8b 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -72,6 +72,7 @@ struct bpf_htab {
u32 n_buckets;
u32 elem_size;
struct bpf_sock_progs progs;
+ struct rcu_head rcu;
};
struct htab_elem {
@@ -89,8 +90,8 @@ enum smap_psock_state {
struct smap_psock_map_entry {
struct list_head list;
struct sock **entry;
- struct htab_elem *hash_link;
- struct bpf_htab *htab;
+ struct htab_elem __rcu *hash_link;
+ struct bpf_htab __rcu *htab;
};
struct smap_psock {
@@ -120,6 +121,7 @@ struct smap_psock {
struct bpf_prog *bpf_parse;
struct bpf_prog *bpf_verdict;
struct list_head maps;
+ spinlock_t maps_lock;
/* Back reference used when sock callback trigger sockmap operations */
struct sock *sock;
@@ -258,16 +260,54 @@ static void bpf_tcp_release(struct sock *sk)
rcu_read_unlock();
}
+static struct htab_elem *lookup_elem_raw(struct hlist_head *head,
+ u32 hash, void *key, u32 key_size)
+{
+ struct htab_elem *l;
+
+ hlist_for_each_entry_rcu(l, head, hash_node) {
+ if (l->hash == hash && !memcmp(&l->key, key, key_size))
+ return l;
+ }
+
+ return NULL;
+}
+
+static inline struct bucket *__select_bucket(struct bpf_htab *htab, u32 hash)
+{
+ return &htab->buckets[hash & (htab->n_buckets - 1)];
+}
+
+static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 hash)
+{
+ return &__select_bucket(htab, hash)->head;
+}
+
static void free_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
{
atomic_dec(&htab->count);
kfree_rcu(l, rcu);
}
+static struct smap_psock_map_entry *psock_map_pop(struct sock *sk,
+ struct smap_psock *psock)
+{
+ struct smap_psock_map_entry *e;
+
+ spin_lock_bh(&psock->maps_lock);
+ e = list_first_entry_or_null(&psock->maps,
+ struct smap_psock_map_entry,
+ list);
+ if (e)
+ list_del(&e->list);
+ spin_unlock_bh(&psock->maps_lock);
+ return e;
+}
+
static void bpf_tcp_close(struct sock *sk, long timeout)
{
void (*close_fun)(struct sock *sk, long timeout);
- struct smap_psock_map_entry *e, *tmp;
+ struct smap_psock_map_entry *e;
struct sk_msg_buff *md, *mtmp;
struct smap_psock *psock;
struct sock *osk;
@@ -286,7 +326,6 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
*/
close_fun = psock->save_close;
- write_lock_bh(&sk->sk_callback_lock);
if (psock->cork) {
free_start_sg(psock->sock, psock->cork);
kfree(psock->cork);
@@ -299,20 +338,38 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
kfree(md);
}
- list_for_each_entry_safe(e, tmp, &psock->maps, list) {
+ e = psock_map_pop(sk, psock);
+ while (e) {
if (e->entry) {
osk = cmpxchg(e->entry, sk, NULL);
if (osk == sk) {
- list_del(&e->list);
smap_release_sock(psock, sk);
}
} else {
- hlist_del_rcu(&e->hash_link->hash_node);
- smap_release_sock(psock, e->hash_link->sk);
- free_htab_elem(e->htab, e->hash_link);
+ struct htab_elem *link = rcu_dereference(e->hash_link);
+ struct bpf_htab *htab = rcu_dereference(e->htab);
+ struct hlist_head *head;
+ struct htab_elem *l;
+ struct bucket *b;
+
+ b = __select_bucket(htab, link->hash);
+ head = &b->head;
+ raw_spin_lock_bh(&b->lock);
+ l = lookup_elem_raw(head,
+ link->hash, link->key,
+ htab->map.key_size);
+ /* If another thread deleted this object skip deletion.
+ * The refcnt on psock may or may not be zero.
+ */
+ if (l) {
+ hlist_del_rcu(&link->hash_node);
+ smap_release_sock(psock, link->sk);
+ free_htab_elem(htab, link);
+ }
+ raw_spin_unlock_bh(&b->lock);
}
+ e = psock_map_pop(sk, psock);
}
- write_unlock_bh(&sk->sk_callback_lock);
rcu_read_unlock();
close_fun(sk, timeout);
}
@@ -1395,7 +1452,9 @@ static void smap_release_sock(struct smap_psock *psock, struct sock *sock)
{
if (refcount_dec_and_test(&psock->refcnt)) {
tcp_cleanup_ulp(sock);
+ write_lock_bh(&sock->sk_callback_lock);
smap_stop_sock(psock, sock);
+ write_unlock_bh(&sock->sk_callback_lock);
clear_bit(SMAP_TX_RUNNING, &psock->state);
rcu_assign_sk_user_data(sock, NULL);
call_rcu_sched(&psock->rcu, smap_destroy_psock);
@@ -1546,6 +1605,7 @@ static struct smap_psock *smap_init_psock(struct sock *sock, int node)
INIT_LIST_HEAD(&psock->maps);
INIT_LIST_HEAD(&psock->ingress);
refcount_set(&psock->refcnt, 1);
+ spin_lock_init(&psock->maps_lock);
rcu_assign_sk_user_data(sock, psock);
sock_hold(sock);
@@ -1607,10 +1667,12 @@ static void smap_list_map_remove(struct smap_psock *psock,
{
struct smap_psock_map_entry *e, *tmp;
+ spin_lock_bh(&psock->maps_lock);
list_for_each_entry_safe(e, tmp, &psock->maps, list) {
if (e->entry == entry)
list_del(&e->list);
}
+ spin_unlock_bh(&psock->maps_lock);
}
static void smap_list_hash_remove(struct smap_psock *psock,
@@ -1618,12 +1680,14 @@ static void smap_list_hash_remove(struct smap_psock *psock,
{
struct smap_psock_map_entry *e, *tmp;
+ spin_lock_bh(&psock->maps_lock);
list_for_each_entry_safe(e, tmp, &psock->maps, list) {
- struct htab_elem *c = e->hash_link;
+ struct htab_elem *c = rcu_dereference(e->hash_link);
if (c == hash_link)
list_del(&e->list);
}
+ spin_unlock_bh(&psock->maps_lock);
}
static void sock_map_free(struct bpf_map *map)
@@ -1649,7 +1713,6 @@ static void sock_map_free(struct bpf_map *map)
if (!sock)
continue;
- write_lock_bh(&sock->sk_callback_lock);
psock = smap_psock_sk(sock);
/* This check handles a racing sock event that can get the
* sk_callback_lock before this case but after xchg happens
@@ -1660,7 +1723,6 @@ static void sock_map_free(struct bpf_map *map)
smap_list_map_remove(psock, &stab->sock_map[i]);
smap_release_sock(psock, sock);
}
- write_unlock_bh(&sock->sk_callback_lock);
}
rcu_read_unlock();
@@ -1709,7 +1771,6 @@ static int sock_map_delete_elem(struct bpf_map *map, void *key)
if (!sock)
return -EINVAL;
- write_lock_bh(&sock->sk_callback_lock);
psock = smap_psock_sk(sock);
if (!psock)
goto out;
@@ -1719,7 +1780,6 @@ static int sock_map_delete_elem(struct bpf_map *map, void *key)
smap_list_map_remove(psock, &stab->sock_map[k]);
smap_release_sock(psock, sock);
out:
- write_unlock_bh(&sock->sk_callback_lock);
return 0;
}
@@ -1800,7 +1860,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
}
}
- write_lock_bh(&sock->sk_callback_lock);
psock = smap_psock_sk(sock);
/* 2. Do not allow inheriting programs if psock exists and has
@@ -1857,7 +1916,9 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
if (err)
goto out_free;
smap_init_progs(psock, verdict, parse);
+ write_lock_bh(&sock->sk_callback_lock);
smap_start_sock(psock, sock);
+ write_unlock_bh(&sock->sk_callback_lock);
}
/* 4. Place psock in sockmap for use and stop any programs on
@@ -1867,9 +1928,10 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
*/
if (map_link) {
e->entry = map_link;
+ spin_lock_bh(&psock->maps_lock);
list_add_tail(&e->list, &psock->maps);
+ spin_unlock_bh(&psock->maps_lock);
}
- write_unlock_bh(&sock->sk_callback_lock);
return err;
out_free:
smap_release_sock(psock, sock);
@@ -1880,7 +1942,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
}
if (tx_msg)
bpf_prog_put(tx_msg);
- write_unlock_bh(&sock->sk_callback_lock);
kfree(e);
return err;
}
@@ -1917,10 +1978,8 @@ static int sock_map_ctx_update_elem(struct bpf_sock_ops_kern *skops,
if (osock) {
struct smap_psock *opsock = smap_psock_sk(osock);
- write_lock_bh(&osock->sk_callback_lock);
smap_list_map_remove(opsock, &stab->sock_map[i]);
smap_release_sock(opsock, osock);
- write_unlock_bh(&osock->sk_callback_lock);
}
out:
return err;
@@ -2091,14 +2150,13 @@ static struct bpf_map *sock_hash_alloc(union bpf_attr *attr)
return ERR_PTR(err);
}
-static inline struct bucket *__select_bucket(struct bpf_htab *htab, u32 hash)
+static void __bpf_htab_free(struct rcu_head *rcu)
{
- return &htab->buckets[hash & (htab->n_buckets - 1)];
-}
+ struct bpf_htab *htab;
-static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 hash)
-{
- return &__select_bucket(htab, hash)->head;
+ htab = container_of(rcu, struct bpf_htab, rcu);
+ bpf_map_area_free(htab->buckets);
+ kfree(htab);
}
static void sock_hash_free(struct bpf_map *map)
@@ -2117,16 +2175,18 @@ static void sock_hash_free(struct bpf_map *map)
*/
rcu_read_lock();
for (i = 0; i < htab->n_buckets; i++) {
- struct hlist_head *head = select_bucket(htab, i);
+ struct bucket *b = __select_bucket(htab, i);
+ struct hlist_head *head;
struct hlist_node *n;
struct htab_elem *l;
+ raw_spin_lock_bh(&b->lock);
+ head = &b->head;
hlist_for_each_entry_safe(l, n, head, hash_node) {
struct sock *sock = l->sk;
struct smap_psock *psock;
hlist_del_rcu(&l->hash_node);
- write_lock_bh(&sock->sk_callback_lock);
psock = smap_psock_sk(sock);
/* This check handles a racing sock event that can get
* the sk_callback_lock before this case but after xchg
@@ -2137,13 +2197,12 @@ static void sock_hash_free(struct bpf_map *map)
smap_list_hash_remove(psock, l);
smap_release_sock(psock, sock);
}
- write_unlock_bh(&sock->sk_callback_lock);
- kfree(l);
+ free_htab_elem(htab, l);
}
+ raw_spin_unlock_bh(&b->lock);
}
rcu_read_unlock();
- bpf_map_area_free(htab->buckets);
- kfree(htab);
+ call_rcu(&htab->rcu, __bpf_htab_free);
}
static struct htab_elem *alloc_sock_hash_elem(struct bpf_htab *htab,
@@ -2170,19 +2229,6 @@ static struct htab_elem *alloc_sock_hash_elem(struct bpf_htab *htab,
return l_new;
}
-static struct htab_elem *lookup_elem_raw(struct hlist_head *head,
- u32 hash, void *key, u32 key_size)
-{
- struct htab_elem *l;
-
- hlist_for_each_entry_rcu(l, head, hash_node) {
- if (l->hash == hash && !memcmp(&l->key, key, key_size))
- return l;
- }
-
- return NULL;
-}
-
static inline u32 htab_map_hash(const void *key, u32 key_len)
{
return jhash(key, key_len, 0);
@@ -2302,9 +2348,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
goto bucket_err;
}
- e->hash_link = l_new;
- e->htab = container_of(map, struct bpf_htab, map);
+ rcu_assign_pointer(e->hash_link, l_new);
+ rcu_assign_pointer(e->htab,
+ container_of(map, struct bpf_htab, map));
+ spin_lock_bh(&psock->maps_lock);
list_add_tail(&e->list, &psock->maps);
+ spin_unlock_bh(&psock->maps_lock);
/* add new element to the head of the list, so that
* concurrent search will find it before old elem
@@ -2374,7 +2423,6 @@ static int sock_hash_delete_elem(struct bpf_map *map, void *key)
struct smap_psock *psock;
hlist_del_rcu(&l->hash_node);
- write_lock_bh(&sock->sk_callback_lock);
psock = smap_psock_sk(sock);
/* This check handles a racing sock event that can get the
* sk_callback_lock before this case but after xchg happens
@@ -2385,7 +2433,6 @@ static int sock_hash_delete_elem(struct bpf_map *map, void *key)
smap_list_hash_remove(psock, l);
smap_release_sock(psock, sock);
}
- write_unlock_bh(&sock->sk_callback_lock);
free_htab_elem(htab, l);
ret = 0;
}
^ permalink raw reply related
* [bpf PATCH v5 4/4] bpf: sockhash, add release routine
From: John Fastabend @ 2018-06-30 13:17 UTC (permalink / raw)
To: ast, daniel, kafai; +Cc: netdev
In-Reply-To: <20180630131506.17344.7833.stgit@john-Precision-Tower-5810>
Add map_release_uref pointer to hashmap ops. This was dropped when
original sockhash code was ported into bpf-next before initial
commit.
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Acked-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/sockmap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 60b6e8b..4fc2cb1 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -2478,6 +2478,7 @@ struct sock *__sock_hash_lookup_elem(struct bpf_map *map, void *key)
.map_get_next_key = sock_hash_get_next_key,
.map_update_elem = sock_hash_update_elem,
.map_delete_elem = sock_hash_delete_elem,
+ .map_release_uref = sock_map_release,
};
BPF_CALL_4(bpf_sock_map_update, struct bpf_sock_ops_kern *, bpf_sock,
^ permalink raw reply related
* Re: [PATCH 0/2 ] cdc_ncm: Handle multicast Ethernet traffic
From: Miguel Rodríguez Pérez @ 2018-06-30 13:43 UTC (permalink / raw)
To: Bjørn Mork; +Cc: linux-usb, netdev
In-Reply-To: <87muvczcot.fsf@miraculix.mork.no>
On 30/06/18 14:22, Bjørn Mork wrote:
> Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:
>
>> Sending again, as the previous try had the wrong subjects. Sorry about that.
>
> No problem really, but please use version numbers so it's easy to see
> which set is most current.
Fine, I'll do so in next revision.
>
> And you didn't address Olivers comment. Why not?
Well, I did, but I forgot to format it as plain text and it didn't reach
the least. I have just resent it.
>
>
> Bjørn
>
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply
* [bpf PATCH 2/2] bpf: sockmap, hash table is RCU so readers do not need locks
From: John Fastabend @ 2018-06-30 13:51 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, kafai
In-Reply-To: <20180630134148.6395.64795.stgit@john-Precision-Tower-5810>
This removes locking from readers of RCU hash table. Its not
necessary.
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
0 files changed
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 63fb047..12ac10a 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -2451,10 +2451,8 @@ struct sock *__sock_hash_lookup_elem(struct bpf_map *map, void *key)
b = __select_bucket(htab, hash);
head = &b->head;
- raw_spin_lock_bh(&b->lock);
l = lookup_elem_raw(head, hash, key, key_size);
sk = l ? l->sk : NULL;
- raw_spin_unlock_bh(&b->lock);
return sk;
}
^ permalink raw reply related
* [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case
From: John Fastabend @ 2018-06-30 13:51 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, kafai
In-Reply-To: <20180630134148.6395.64795.stgit@john-Precision-Tower-5810>
The current code, in the error path of sock_hash_ctx_update_elem,
checks if the sock has a psock in the user data and if so decrements
the reference count of the psock. However, if the error happens early
in the error path we may have never incremented the psock reference
count and if the psock exists because the sock is in another map then
we may inadvertently decrement the reference count.
Fix this by making the error path only call smap_release_sock if the
error happens after the increment.
Reported-by: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
0 files changed
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 4fc2cb1..63fb047 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1896,7 +1896,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
e = kzalloc(sizeof(*e), GFP_ATOMIC | __GFP_NOWARN);
if (!e) {
err = -ENOMEM;
- goto out_progs;
+ goto out_free;
}
}
@@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
if (err)
goto err;
- /* bpf_map_update_elem() can be called in_irq() */
+ psock = smap_psock_sk(sock);
+ if (unlikely(!psock)) {
+ err = -EINVAL;
+ goto err;
+ }
+
raw_spin_lock_bh(&b->lock);
l_old = lookup_elem_raw(head, hash, key, key_size);
if (l_old && map_flags == BPF_NOEXIST) {
@@ -2342,12 +2347,6 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
goto bucket_err;
}
- psock = smap_psock_sk(sock);
- if (unlikely(!psock)) {
- err = -EINVAL;
- goto bucket_err;
- }
-
rcu_assign_pointer(e->hash_link, l_new);
rcu_assign_pointer(e->htab,
container_of(map, struct bpf_htab, map));
@@ -2370,12 +2369,10 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
raw_spin_unlock_bh(&b->lock);
return 0;
bucket_err:
+ smap_release_sock(psock, sock);
raw_spin_unlock_bh(&b->lock);
err:
kfree(e);
- psock = smap_psock_sk(sock);
- if (psock)
- smap_release_sock(psock, sock);
return err;
}
^ permalink raw reply related
* [bpf PATCH 0/2] sockmap, syzbot fix error path and RCU fix
From: John Fastabend @ 2018-06-30 13:51 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, kafai
This applies on top of "BPF fixes for sockhash" I just didn't
want to confuse that series yet again by re-ordering/adding
these patches in it
I missed fixing the error path in the sockhash code to align with
supporting socks in multiple maps. Simply checking if the psock is
present does not mean we can decrement the reference count because
it could be part of another map. Fix this by cleaning up the error
path so this situation does not happen.
As far as I know this should be the last fix to the fallout from
relaxing the single map restriction. Sorry about the multiple fixes
but these patches were all written before the initial submission then
converted and I missed this detail. But at least we caught these early
in the net cycle. Will continue reviewing/testing however to see if
we catch anything else.
Also we need one more series to check ESTABLISH state as Eric noted.
That will be sent out shortly just going over the patches once more.
The ESTABLISH/unhash fix is also needed in kTLS.
---
John Fastabend (2):
bpf: sockmap, error path can not release psock in multi-map case
bpf: sockmap, hash table is RCU so readers do not need locks
0 files changed
^ permalink raw reply
* Re: [PATCH 1/2] cdc_ncm: Hook into usbnet_change_mtu respecting usbnet,driver_info
From: Miguel Rodríguez Pérez @ 2018-06-30 14:22 UTC (permalink / raw)
To: Bjørn Mork; +Cc: linux-usb, netdev
In-Reply-To: <87vaa0zdnx.fsf@miraculix.mork.no>
On 30/06/18 14:01, Bjørn Mork wrote:
> Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:
>
>> Change the way cdc_ncm_change_mtu hooks into the netdev_ops
>> structure so that changes into usbnet driver_info operations
>> can be respected. Without this, is was not possible to hook
>> into usbnet_set_rx_mode.
>
> Please export usbnet_set_rx_mode instead, so that cdc_ncm_netdev_ops
> can be kept const.
>
I don't see how this would help. From my point of view, the problem is
that cdc_ncm driver should not be setting net_device_ops in the first
place, as that is usbnet.c job. Sadly there seems to be no way currently
for cdc_ncm to be notified of mtu changes. This was the least intrusive
way I could think of to respect usbnet's net_device_ops structure while
keeping cdc_ncm notified of mtu changes. Without this change,
set_rx_mode was not getting called for cdc_ncm even after modifying
struct driver_info.
>
> Bjørn
>
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply
* Re: [PATCH 2/2] cdc_ncm: Admit multicast traffic
From: Miguel Rodríguez Pérez @ 2018-06-30 14:22 UTC (permalink / raw)
To: Bjørn Mork; +Cc: linux-usb, netdev
In-Reply-To: <87r2kozcx0.fsf@miraculix.mork.no>
On 30/06/18 14:17, Bjørn Mork wrote:
> Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:
>
>> +static void cdc_ncm_update_filter(struct usbnet *dev)
>> +{
>> + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
>> + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
>> + struct net_device *net = dev->net;
>
> Why not change usbnet_cdc_update_filter to get the interface number from
> dev->intf instead? Then you can export it and reuse it here instead of
> creating a copy. And it will also work for any other usbnet minidriver.
Well, I just wanted to limit my changes to the minimum, as I lack
experience with kernel development. Where should I declare
usbnet_cdc_update then? Would linux/usb/cdc.h the right place? Is it
enough to do EXPORT_GPL_SYMBOL(usbnet_cdc_update_filter) in cdc_ether.c
for it to be loaded automatically from cdc_ncm.c?
>
> I.e. change this:
>
> static void usbnet_cdc_update_filter(struct usbnet *dev)
> {
> struct cdc_state *info = (void *) &dev->data;
> struct usb_interface *intf = info->control;
>
> into:
>
> static void usbnet_cdc_update_filter(struct usbnet *dev)
> {
> struct usb_interface *intf = dev->intf;
>
>
> or simply use dev->intf to deref directly into an interface number, like
> you do in your version. The local 'intf' variable doesn't do much
> good here.
>
Ok, done.
>
>
>> +
>> + u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
>> + | USB_CDC_PACKET_TYPE_BROADCAST;
>> +
>> + /* filtering on the device is an optional feature and not worth
>> + * the hassle so we just roughly care about snooping and if any
>> + * multicast is requested, we take every multicast
>> + */
>> + if (net->flags & IFF_PROMISC)
>> + cdc_filter |= USB_CDC_PACKET_TYPE_PROMISCUOUS;
>> + if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI))
>> + cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST;
>> +
>> + usbnet_write_cmd(dev,
>
> This is a nice change. It should be probably done in
> usbnet_cdc_update_filter in any case. Unless there is some reason it
> doesn't alreay use usbnet_write_cmd?
I don't know of any, so I'll keep it changed.
>
>> + USB_CDC_SET_ETHERNET_PACKET_FILTER,
>> + USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
>> + cdc_filter,
>> + iface_no,
>> + NULL,
>> + 0);
>> +}
>> +
>> static const struct ethtool_ops cdc_ncm_ethtool_ops = {
>> .get_link = usbnet_get_link,
>> .nway_reset = usbnet_nway_reset,
>> @@ -1652,6 +1679,7 @@ static const struct driver_info cdc_ncm_info = {
>> .status = cdc_ncm_status,
>> .rx_fixup = cdc_ncm_rx_fixup,
>> .tx_fixup = cdc_ncm_tx_fixup,
>> + .set_rx_mode = cdc_ncm_update_filter,
>> };
>>
>> /* Same as cdc_ncm_info, but with FLAG_WWAN */
>
>
> As the comment indicates: There are more 'struct driver_info' variants
> here. Please add .set_rx_mode to all of them, unless you have a reason
> not to?
I simply lack access to the hardware to test and I didn't want to break
anybody else's hardware. In any case, the change seems quite safe, so I
modified them all too.
I will for your comments before sending the updated version of the patches.
>
>
> Bjørn
>
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply
* [PATCH net] net: fix use-after-free in GRO with ESP
From: Sabrina Dubroca @ 2018-06-30 15:38 UTC (permalink / raw)
To: netdev; +Cc: Sabrina Dubroca, Stefano Brivio, Steffen Klassert
Since the addition of GRO for ESP, gro_receive can consume the skb and
return -EINPROGRESS. In that case, the lower layer GRO handler cannot
touch the skb anymore.
Commit 5f114163f2f5 ("net: Add a skb_gro_flush_final helper.") converted
some of the gro_receive handlers that can lead to ESP's gro_receive so
that they wouldn't access the skb when -EINPROGRESS is returned, but
missed other spots, mainly in tunneling protocols.
This patch finishes the conversion to using skb_gro_flush_final(), and
adds a new helper, skb_gro_flush_final_remcsum(), used in VXLAN and
GUE.
Fixes: 5f114163f2f5 ("net: Add a skb_gro_flush_final helper.")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
---
drivers/net/geneve.c | 2 +-
drivers/net/vxlan.c | 4 +---
include/linux/netdevice.h | 20 ++++++++++++++++++++
net/8021q/vlan.c | 2 +-
net/ipv4/fou.c | 4 +---
net/ipv4/gre_offload.c | 2 +-
net/ipv4/udp_offload.c | 2 +-
7 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 750eaa53bf0c..ada33c2d9ac2 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -476,7 +476,7 @@ static struct sk_buff **geneve_gro_receive(struct sock *sk,
out_unlock:
rcu_read_unlock();
out:
- NAPI_GRO_CB(skb)->flush |= flush;
+ skb_gro_flush_final(skb, pp, flush);
return pp;
}
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index aee0e60471f1..f6bb1d54d4bd 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -623,9 +623,7 @@ static struct sk_buff **vxlan_gro_receive(struct sock *sk,
flush = 0;
out:
- skb_gro_remcsum_cleanup(skb, &grc);
- skb->remcsum_offload = 0;
- NAPI_GRO_CB(skb)->flush |= flush;
+ skb_gro_flush_final_remcsum(skb, pp, flush, &grc);
return pp;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3ec9850c7936..3d0cc0b5cec2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2789,11 +2789,31 @@ static inline void skb_gro_flush_final(struct sk_buff *skb, struct sk_buff **pp,
if (PTR_ERR(pp) != -EINPROGRESS)
NAPI_GRO_CB(skb)->flush |= flush;
}
+static inline void skb_gro_flush_final_remcsum(struct sk_buff *skb,
+ struct sk_buff **pp,
+ int flush,
+ struct gro_remcsum *grc)
+{
+ if (PTR_ERR(pp) != -EINPROGRESS) {
+ NAPI_GRO_CB(skb)->flush |= flush;
+ skb_gro_remcsum_cleanup(skb, grc);
+ skb->remcsum_offload = 0;
+ }
+}
#else
static inline void skb_gro_flush_final(struct sk_buff *skb, struct sk_buff **pp, int flush)
{
NAPI_GRO_CB(skb)->flush |= flush;
}
+static inline void skb_gro_flush_final_remcsum(struct sk_buff *skb,
+ struct sk_buff **pp,
+ int flush,
+ struct gro_remcsum *grc)
+{
+ NAPI_GRO_CB(skb)->flush |= flush;
+ skb_gro_remcsum_cleanup(skb, grc);
+ skb->remcsum_offload = 0;
+}
#endif
static inline int dev_hard_header(struct sk_buff *skb, struct net_device *dev,
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 73a65789271b..8ccee3d01822 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -693,7 +693,7 @@ static struct sk_buff **vlan_gro_receive(struct sk_buff **head,
out_unlock:
rcu_read_unlock();
out:
- NAPI_GRO_CB(skb)->flush |= flush;
+ skb_gro_flush_final(skb, pp, flush);
return pp;
}
diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
index 1540db65241a..c9ec1603666b 100644
--- a/net/ipv4/fou.c
+++ b/net/ipv4/fou.c
@@ -448,9 +448,7 @@ static struct sk_buff **gue_gro_receive(struct sock *sk,
out_unlock:
rcu_read_unlock();
out:
- NAPI_GRO_CB(skb)->flush |= flush;
- skb_gro_remcsum_cleanup(skb, &grc);
- skb->remcsum_offload = 0;
+ skb_gro_flush_final_remcsum(skb, pp, flush, &grc);
return pp;
}
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index 1859c473b21a..6a7d980105f6 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -223,7 +223,7 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
out_unlock:
rcu_read_unlock();
out:
- NAPI_GRO_CB(skb)->flush |= flush;
+ skb_gro_flush_final(skb, pp, flush);
return pp;
}
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 92dc9e5a7ff3..69c54540d5b4 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -394,7 +394,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
out_unlock:
rcu_read_unlock();
out:
- NAPI_GRO_CB(skb)->flush |= flush;
+ skb_gro_flush_final(skb, pp, flush);
return pp;
}
EXPORT_SYMBOL(udp_gro_receive);
--
2.18.0
^ permalink raw reply related
* Re: [net-next 01/12] net/mlx5e: Add UDP GSO support
From: Boris Pismenny @ 2018-06-30 16:06 UTC (permalink / raw)
To: Willem de Bruijn, Saeed Mahameed
Cc: David Miller, Network Development, ogerlitz, yossiku,
Alexander Duyck
In-Reply-To: <CAF=yD-LHN6GS7aGBm3enQDJNrvrUp6aGGMcr7bHOR0PNrLqKJA@mail.gmail.com>
On 06/30/18 01:19, Willem de Bruijn wrote:
> On Fri, Jun 29, 2018 at 2:24 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
>> From: Boris Pismenny <borisp@mellanox.com>
>>
>> This patch enables UDP GSO support. We enable this by using two WQEs
>> the first is a UDP LSO WQE for all segments with equal length, and the
>> second is for the last segment in case it has different length.
>> Due to HW limitation, before sending, we must adjust the packet length fields.
>>
>> We measure performance between two Intel(R) Xeon(R) CPU E5-2643 v2 @3.50GHz
>> machines connected back-to-back with Connectx4-Lx (40Gbps) NICs.
>> We compare single stream UDP, UDP GSO and UDP GSO with offload.
>> Performance:
>> | MSS (bytes) | Throughput (Gbps) | CPU utilization (%)
>> UDP GSO offload | 1472 | 35.6 | 8%
>> UDP GSO | 1472 | 25.5 | 17%
>> UDP | 1472 | 10.2 | 17%
>> UDP GSO offload | 1024 | 35.6 | 8%
>> UDP GSO | 1024 | 19.2 | 17%
>> UDP | 1024 | 5.7 | 17%
>> UDP GSO offload | 512 | 33.8 | 16%
>> UDP GSO | 512 | 10.4 | 17%
>> UDP | 512 | 3.5 | 17%
> Very nice results :)
Thanks. We expect to have 100Gbps results by netdev0x12.
>> +static void mlx5e_udp_gso_prepare_last_skb(struct sk_buff *skb,
>> + struct sk_buff *nskb,
>> + int remaining)
>> +{
>> + int bytes_needed = remaining, remaining_headlen, remaining_page_offset;
>> + int headlen = skb_transport_offset(skb) + sizeof(struct udphdr);
>> + int payload_len = remaining + sizeof(struct udphdr);
>> + int k = 0, i, j;
>> +
>> + skb_copy_bits(skb, 0, nskb->data, headlen);
>> + nskb->dev = skb->dev;
>> + skb_reset_mac_header(nskb);
>> + skb_set_network_header(nskb, skb_network_offset(skb));
>> + skb_set_transport_header(nskb, skb_transport_offset(skb));
>> + skb_set_tail_pointer(nskb, headlen);
>> +
>> + /* How many frags do we need? */
>> + for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
>> + bytes_needed -= skb_frag_size(&skb_shinfo(skb)->frags[i]);
>> + k++;
>> + if (bytes_needed <= 0)
>> + break;
>> + }
>> +
>> + /* Fill the first frag and split it if necessary */
>> + j = skb_shinfo(skb)->nr_frags - k;
>> + remaining_page_offset = -bytes_needed;
>> + skb_fill_page_desc(nskb, 0,
>> + skb_shinfo(skb)->frags[j].page.p,
>> + skb_shinfo(skb)->frags[j].page_offset + remaining_page_offset,
>> + skb_shinfo(skb)->frags[j].size - remaining_page_offset);
>> +
>> + skb_frag_ref(skb, j);
>> +
>> + /* Fill the rest of the frags */
>> + for (i = 1; i < k; i++) {
>> + j = skb_shinfo(skb)->nr_frags - k + i;
>> +
>> + skb_fill_page_desc(nskb, i,
>> + skb_shinfo(skb)->frags[j].page.p,
>> + skb_shinfo(skb)->frags[j].page_offset,
>> + skb_shinfo(skb)->frags[j].size);
>> + skb_frag_ref(skb, j);
>> + }
>> + skb_shinfo(nskb)->nr_frags = k;
>> +
>> + remaining_headlen = remaining - skb->data_len;
>> +
>> + /* headlen contains remaining data? */
>> + if (remaining_headlen > 0)
>> + skb_copy_bits(skb, skb->len - remaining, nskb->data + headlen,
>> + remaining_headlen);
>> + nskb->len = remaining + headlen;
>> + nskb->data_len = payload_len - sizeof(struct udphdr) +
>> + max_t(int, 0, remaining_headlen);
>> + nskb->protocol = skb->protocol;
>> + if (nskb->protocol == htons(ETH_P_IP)) {
>> + ip_hdr(nskb)->id = htons(ntohs(ip_hdr(nskb)->id) +
>> + skb_shinfo(skb)->gso_segs);
>> + ip_hdr(nskb)->tot_len =
>> + htons(payload_len + sizeof(struct iphdr));
>> + } else {
>> + ipv6_hdr(nskb)->payload_len = htons(payload_len);
>> + }
>> + udp_hdr(nskb)->len = htons(payload_len);
>> + skb_shinfo(nskb)->gso_size = 0;
>> + nskb->ip_summed = skb->ip_summed;
>> + nskb->csum_start = skb->csum_start;
>> + nskb->csum_offset = skb->csum_offset;
>> + nskb->queue_mapping = skb->queue_mapping;
>> +}
>> +
>> +/* might send skbs and update wqe and pi */
>> +struct sk_buff *mlx5e_udp_gso_handle_tx_skb(struct net_device *netdev,
>> + struct mlx5e_txqsq *sq,
>> + struct sk_buff *skb,
>> + struct mlx5e_tx_wqe **wqe,
>> + u16 *pi)
>> +{
>> + int payload_len = skb_shinfo(skb)->gso_size + sizeof(struct udphdr);
>> + int headlen = skb_transport_offset(skb) + sizeof(struct udphdr);
>> + int remaining = (skb->len - headlen) % skb_shinfo(skb)->gso_size;
>> + struct sk_buff *nskb;
>> +
>> + if (skb->protocol == htons(ETH_P_IP))
>> + ip_hdr(skb)->tot_len = htons(payload_len + sizeof(struct iphdr));
>> + else
>> + ipv6_hdr(skb)->payload_len = htons(payload_len);
>> + udp_hdr(skb)->len = htons(payload_len);
>> + if (!remaining)
>> + return skb;
>> +
>> + nskb = alloc_skb(max_t(int, headlen, headlen + remaining - skb->data_len), GFP_ATOMIC);
>> + if (unlikely(!nskb)) {
>> + sq->stats->dropped++;
>> + return NULL;
>> + }
>> +
>> + mlx5e_udp_gso_prepare_last_skb(skb, nskb, remaining);
>> +
>> + skb_shinfo(skb)->gso_segs--;
>> + pskb_trim(skb, skb->len - remaining);
>> + mlx5e_sq_xmit(sq, skb, *wqe, *pi);
>> + mlx5e_sq_fetch_wqe(sq, wqe, pi);
>> + return nskb;
>> +}
> The device driver seems to be implementing the packet split here
> similar to NETIF_F_GSO_PARTIAL. When advertising the right flag, the
> stack should be able to do that for you and pass two packets to the
> driver.
We've totally missed that!
Thank you. I'll fix and resubmit.
^ permalink raw reply
* [PATCH net-next 1/5] net: gemini: Look up L3 maxlen from table
From: Linus Walleij @ 2018-06-30 16:18 UTC (permalink / raw)
To: netdev, David S . Miller, Michał Mirosław
Cc: Janos Laube, Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Linus Walleij
The code to calculate the hardware register enumerator
for the maximum L3 length isn't entirely simple to read.
Use the existing defines and rewrite the function into a
table look-up.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/ethernet/cortina/gemini.c | 61 ++++++++++++++++++++-------
1 file changed, 46 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 6d7404f66f84..8fc31723f700 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -401,26 +401,57 @@ static int gmac_setup_phy(struct net_device *netdev)
return 0;
}
-static int gmac_pick_rx_max_len(int max_l3_len)
-{
- /* index = CONFIG_MAXLEN_XXX values */
- static const int max_len[8] = {
- 1536, 1518, 1522, 1542,
- 9212, 10236, 1518, 1518
- };
- int i, n = 5;
+/* The maximum frame length is not logically enumerated in the
+ * hardware, so we do a table lookup to find the applicable max
+ * frame length.
+ */
+struct gmac_max_framelen {
+ unsigned int max_l3_len;
+ u8 val;
+};
- max_l3_len += ETH_HLEN + VLAN_HLEN;
+static const struct gmac_max_framelen gmac_maxlens[] = {
+ {
+ .max_l3_len = 1518,
+ .val = CONFIG0_MAXLEN_1518,
+ },
+ {
+ .max_l3_len = 1522,
+ .val = CONFIG0_MAXLEN_1522,
+ },
+ {
+ .max_l3_len = 1536,
+ .val = CONFIG0_MAXLEN_1536,
+ },
+ {
+ .max_l3_len = 1542,
+ .val = CONFIG0_MAXLEN_1542,
+ },
+ {
+ .max_l3_len = 9212,
+ .val = CONFIG0_MAXLEN_9k,
+ },
+ {
+ .max_l3_len = 10236,
+ .val = CONFIG0_MAXLEN_10k,
+ },
+};
+
+static int gmac_pick_rx_max_len(unsigned int max_l3_len)
+{
+ const struct gmac_max_framelen *maxlen;
+ int maxtot;
+ int i;
- if (max_l3_len > max_len[n])
- return -1;
+ maxtot = max_l3_len + ETH_HLEN + VLAN_HLEN;
- for (i = 0; i < 5; i++) {
- if (max_len[i] >= max_l3_len && max_len[i] < max_len[n])
- n = i;
+ for (i = 0; i < ARRAY_SIZE(gmac_maxlens); i++) {
+ maxlen = &gmac_maxlens[i];
+ if (maxtot <= maxlen->max_l3_len)
+ return maxlen->val;
}
- return n;
+ return -1;
}
static int gmac_init(struct net_device *netdev)
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 2/5] net: gemini: Improve connection prints
From: Linus Walleij @ 2018-06-30 16:18 UTC (permalink / raw)
To: netdev, David S . Miller, Michał Mirosław
Cc: Janos Laube, Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Linus Walleij
In-Reply-To: <20180630161806.24312-1-linus.walleij@linaro.org>
Instead of just specify that a PHY is connected at some
speed, also specify which one. This is helpful with several
PHYs on the system.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/ethernet/cortina/gemini.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 8fc31723f700..b49ed8964026 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -300,23 +300,26 @@ static void gmac_speed_set(struct net_device *netdev)
status.bits.speed = GMAC_SPEED_1000;
if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
status.bits.mii_rmii = GMAC_PHY_RGMII_1000;
- netdev_info(netdev, "connect to RGMII @ 1Gbit\n");
+ netdev_info(netdev, "connect %s to RGMII @ 1Gbit\n",
+ phydev_name(phydev));
break;
case 100:
status.bits.speed = GMAC_SPEED_100;
if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
status.bits.mii_rmii = GMAC_PHY_RGMII_100_10;
- netdev_info(netdev, "connect to RGMII @ 100 Mbit\n");
+ netdev_info(netdev, "connect %s to RGMII @ 100 Mbit\n",
+ phydev_name(phydev));
break;
case 10:
status.bits.speed = GMAC_SPEED_10;
if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
status.bits.mii_rmii = GMAC_PHY_RGMII_100_10;
- netdev_info(netdev, "connect to RGMII @ 10 Mbit\n");
+ netdev_info(netdev, "connect %s to RGMII @ 10 Mbit\n",
+ phydev_name(phydev));
break;
default:
- netdev_warn(netdev, "Not supported PHY speed (%d)\n",
- phydev->speed);
+ netdev_warn(netdev, "Unsupported PHY speed (%d) on %s\n",
+ phydev->speed, phydev_name(phydev));
}
if (phydev->duplex == DUPLEX_FULL) {
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 3/5] net: gemini: Allow multiple ports to instantiate
From: Linus Walleij @ 2018-06-30 16:18 UTC (permalink / raw)
To: netdev, David S . Miller, Michał Mirosław
Cc: Janos Laube, Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Linus Walleij
In-Reply-To: <20180630161806.24312-1-linus.walleij@linaro.org>
The code was not tested with two ports actually in use at
the same time. (I blame this on lack of actual hardware using
that feature.) Now after locating a system using both ports,
add necessary fix to make both ports come up.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/ethernet/cortina/gemini.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index b49ed8964026..8d192fcd51c8 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1787,7 +1787,10 @@ static int gmac_open(struct net_device *netdev)
phy_start(netdev->phydev);
err = geth_resize_freeq(port);
- if (err) {
+ /* It's fine if it's just busy, the other port has set up
+ * the freeq in that case.
+ */
+ if (err && (err != -EBUSY)) {
netdev_err(netdev, "could not resize freeq\n");
goto err_stop_phy;
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 4/5] net: gemini: Move main init to port
From: Linus Walleij @ 2018-06-30 16:18 UTC (permalink / raw)
To: netdev, David S . Miller, Michał Mirosław
Cc: Janos Laube, Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Linus Walleij
In-Reply-To: <20180630161806.24312-1-linus.walleij@linaro.org>
The initialization sequence for the ethernet, setting up
interrupt routing and such things, need to be done after
both the ports are clocked and reset. Before this the
config will not "take". Move the initialization to the
port probe function and keep track of init status in
the state.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/ethernet/cortina/gemini.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 8d192fcd51c8..79324bbfd768 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -146,6 +146,7 @@ struct gemini_ethernet {
void __iomem *base;
struct gemini_ethernet_port *port0;
struct gemini_ethernet_port *port1;
+ bool initialized;
spinlock_t irq_lock; /* Locks IRQ-related registers */
unsigned int freeq_order;
@@ -2301,6 +2302,14 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
static void gemini_ethernet_init(struct gemini_ethernet *geth)
{
+ /* Only do this once both ports are online */
+ if (geth->initialized)
+ return;
+ if (geth->port0 && geth->port1)
+ geth->initialized = true;
+ else
+ return;
+
writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_0_REG);
writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_1_REG);
writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_2_REG);
@@ -2447,6 +2456,10 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
geth->port0 = port;
else
geth->port1 = port;
+
+ /* This will just be done once both ports are up and reset */
+ gemini_ethernet_init(geth);
+
platform_set_drvdata(pdev, port);
/* Set up and register the netdev */
@@ -2564,7 +2577,6 @@ static int gemini_ethernet_probe(struct platform_device *pdev)
spin_lock_init(&geth->irq_lock);
spin_lock_init(&geth->freeq_lock);
- gemini_ethernet_init(geth);
/* The children will use this */
platform_set_drvdata(pdev, geth);
@@ -2577,8 +2589,8 @@ static int gemini_ethernet_remove(struct platform_device *pdev)
{
struct gemini_ethernet *geth = platform_get_drvdata(pdev);
- gemini_ethernet_init(geth);
geth_cleanup_freeq(geth);
+ geth->initialized = false;
return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 5/5] net: gemini: Indicate that we can handle jumboframes
From: Linus Walleij @ 2018-06-30 16:18 UTC (permalink / raw)
To: netdev, David S . Miller, Michał Mirosław
Cc: Janos Laube, Paulius Zaleckas, linux-arm-kernel, Hans Ulli Kroll,
Florian Fainelli, Linus Walleij
In-Reply-To: <20180630161806.24312-1-linus.walleij@linaro.org>
The hardware supposedly handles frames up to 10236 bytes and
implements .ndo_change_mtu() so accept 10236 minus the ethernet
header for a VLAN tagged frame on the netdevices.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/ethernet/cortina/gemini.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 79324bbfd768..ae475393e4ac 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2473,6 +2473,11 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
netdev->hw_features = GMAC_OFFLOAD_FEATURES;
netdev->features |= GMAC_OFFLOAD_FEATURES | NETIF_F_GRO;
+ /* We can handle jumbo frames up to 10236 bytes so, let's accept
+ * payloads of 10236 bytes minus VLAN and ethernet header
+ */
+ netdev->min_mtu = 256;
+ netdev->max_mtu = 10236 - VLAN_ETH_HLEN;
port->freeq_refill = 0;
netif_napi_add(netdev, &port->napi, gmac_napi_poll,
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/3] usbnet: Admit multicast traffic in cdc ncm devices
From: Miguel Rodríguez Pérez @ 2018-06-30 17:27 UTC (permalink / raw)
To: linux-usb, netdev
I have reworked the patches according to the previous round of revisions.
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply
* [PATCH v2 2/4] usbnet: Export usbnet_cdc_update_filter
From: Miguel Rodríguez Pérez @ 2018-06-30 17:31 UTC (permalink / raw)
To: linux-usb, netdev
This one exports usbnet_cdc_update_filter so that it can be reused by
cdc_ncm or any other usbnet driver.
---
drivers/net/usb/cdc_ether.c | 3 ++-
include/linux/usb/usbnet.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 815ed0dc18fe..54472ab77b90 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -75,7 +75,7 @@ static const u8 mbm_guid[16] = {
0xa6, 0x07, 0xc0, 0xff, 0xcb, 0x7e, 0x39, 0x2a,
};
-static void usbnet_cdc_update_filter(struct usbnet *dev)
+void usbnet_cdc_update_filter(struct usbnet *dev)
{
struct net_device *net = dev->net;
@@ -99,6 +99,7 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
NULL,
0);
}
+EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
/* probes control interface, claims data interface, collects the bulk
* endpoints, activates data interface (if needed), maybe sets MTU.
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index e2ec3582e549..7821cf1dcd60 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -286,4 +286,5 @@ extern void usbnet_update_max_qlen(struct usbnet *dev);
extern void usbnet_get_stats64(struct net_device *dev,
struct rtnl_link_stats64 *stats);
+extern void usbnet_cdc_update_filter(struct usbnet *);
#endif /* __LINUX_USB_USBNET_H */
--
2.17.1
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply related
* [PATCH v2 1/4] Simplify usbnet_cdc_update_filter
From: Miguel Rodríguez Pérez @ 2018-06-30 17:32 UTC (permalink / raw)
To: linux-usb, netdev
Remove some unneeded varibles to make the code easier to read
and, replace the generic usb_control_msg function for the
more specific usbnet_write_cmd.
---
drivers/net/usb/cdc_ether.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 178b956501a7..815ed0dc18fe 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -77,9 +77,7 @@ static const u8 mbm_guid[16] = {
static void usbnet_cdc_update_filter(struct usbnet *dev)
{
- struct cdc_state *info = (void *) &dev->data;
- struct usb_interface *intf = info->control;
- struct net_device *net = dev->net;
+ struct net_device *net = dev->net;
u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
| USB_CDC_PACKET_TYPE_BROADCAST;
@@ -93,16 +91,13 @@ static void usbnet_cdc_update_filter(struct usbnet *dev)
if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI))
cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST;
- usb_control_msg(dev->udev,
- usb_sndctrlpipe(dev->udev, 0),
+ usbnet_write_cmd(dev,
USB_CDC_SET_ETHERNET_PACKET_FILTER,
- USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
cdc_filter,
- intf->cur_altsetting->desc.bInterfaceNumber,
+ dev->intf->cur_altsetting->desc.bInterfaceNumber,
NULL,
- 0,
- USB_CTRL_SET_TIMEOUT
- );
+ 0);
}
/* probes control interface, claims data interface, collects the bulk
--
2.17.1
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply related
* [PATCH v2 3/4] Replace the way cdc_ncm hooks into usbnet_change_mtu
From: Miguel Rodríguez Pérez @ 2018-06-30 17:32 UTC (permalink / raw)
To: linux-usb, netdev
Previously cdc_ncm overwrited netdev_ops used by usbnet
thus preventing hooking into set_rx_mode. This patch
preserves usbnet hooks into netdev_ops, and add an
additional one for change_mtu needed by cdc_ncm.
---
drivers/net/usb/cdc_ncm.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 9e1b74590682..d6b51e2b9495 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -750,16 +750,7 @@ int cdc_ncm_change_mtu(struct net_device *net, int
new_mtu)
}
EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu);
-static const struct net_device_ops cdc_ncm_netdev_ops = {
- .ndo_open = usbnet_open,
- .ndo_stop = usbnet_stop,
- .ndo_start_xmit = usbnet_start_xmit,
- .ndo_tx_timeout = usbnet_tx_timeout,
- .ndo_get_stats64 = usbnet_get_stats64,
- .ndo_change_mtu = cdc_ncm_change_mtu,
- .ndo_set_mac_address = eth_mac_addr,
- .ndo_validate_addr = eth_validate_addr,
-};
+static struct net_device_ops cdc_ncm_netdev_ops;
int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf,
u8 data_altsetting, int drvflags)
{
@@ -939,6 +930,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
dev->net->sysfs_groups[0] = &cdc_ncm_sysfs_attr_group;
/* must handle MTU changes */
+ cdc_ncm_netdev_ops = *dev->net->netdev_ops;
+ cdc_ncm_netdev_ops.ndo_change_mtu = cdc_ncm_change_mtu;
dev->net->netdev_ops = &cdc_ncm_netdev_ops;
dev->net->max_mtu = cdc_ncm_max_dgram_size(dev) - cdc_ncm_eth_hlen(dev);
--
2.17.1
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply related
* [PATCH v2 4/4] Hook into set_rx_mode to admit multicast traffic
From: Miguel Rodríguez Pérez @ 2018-06-30 17:32 UTC (permalink / raw)
To: linux-usb, netdev
We set set_rx_mode to usbnet_cdc_update_filter provided
by cdc_ether that simply admits all multicast traffic
if there is more than one multicast filter configured.
---
drivers/net/usb/cdc_ncm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index d6b51e2b9495..fda0af0b5d3c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1652,6 +1652,7 @@ static const struct driver_info cdc_ncm_info = {
.status = cdc_ncm_status,
.rx_fixup = cdc_ncm_rx_fixup,
.tx_fixup = cdc_ncm_tx_fixup,
+ .set_rx_mode = usbnet_cdc_update_filter,
};
/* Same as cdc_ncm_info, but with FLAG_WWAN */
@@ -1665,6 +1666,7 @@ static const struct driver_info wwan_info = {
.status = cdc_ncm_status,
.rx_fixup = cdc_ncm_rx_fixup,
.tx_fixup = cdc_ncm_tx_fixup,
+ .set_rx_mode = usbnet_cdc_update_filter,
};
/* Same as wwan_info, but with FLAG_NOARP */
@@ -1678,6 +1680,7 @@ static const struct driver_info wwan_noarp_info = {
.status = cdc_ncm_status,
.rx_fixup = cdc_ncm_rx_fixup,
.tx_fixup = cdc_ncm_tx_fixup,
+ .set_rx_mode = usbnet_cdc_update_filter,
};
static const struct usb_device_id cdc_devs[] = {
--
2.17.1
--
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo
^ permalink raw reply related
* Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives
From: Neal Cardwell @ 2018-06-30 18:23 UTC (permalink / raw)
To: Lawrence Brakmo
Cc: Netdev, Kernel Team, bmatheny, ast, Yuchung Cheng, Steve Ibanez,
Eric Dumazet
In-Reply-To: <20180630014815.2881895-3-brakmo@fb.com>
On Fri, Jun 29, 2018 at 9:48 PM Lawrence Brakmo <brakmo@fb.com> wrote:
>
> We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
> problem is triggered when the last packet of a request arrives CE
> marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
> to 1 (because there are no packets in flight). When the 1st packet of
> the next request arrives it was sometimes delayed adding up to 40ms to
> the latency.
>
> This patch insures that CWR makred data packets arriving will be acked
> immediately.
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
Thanks, Larry. Ensuring that CWR-marked data packets arriving will be
acked immediately sounds like a good goal to me.
I am wondering if we can have a simpler fix.
The dctcp_ce_state_0_to_1() code is setting the TCP_ECN_DEMAND_CWR
bit in ecn_flags, which disables the code in __tcp_ecn_check_ce() that
would have otherwise used the tcp_enter_quickack_mode() mechanism to
force an ACK:
__tcp_ecn_check_ce():
...
case INET_ECN_CE:
if (tcp_ca_needs_ecn(sk))
tcp_ca_event(sk, CA_EVENT_ECN_IS_CE);
// -> dctcp_ce_state_0_to_1()
// -> tp->ecn_flags |= TCP_ECN_DEMAND_CWR;
if (!(tp->ecn_flags & TCP_ECN_DEMAND_CWR)) {
/* Better not delay acks, sender can have a very low cwnd */
tcp_enter_quickack_mode(sk, 1);
tp->ecn_flags |= TCP_ECN_DEMAND_CWR;
}
tp->ecn_flags |= TCP_ECN_SEEN;
break;
So it seems like the bug here may be that dctcp_ce_state_0_to_1() is
setting the TCP_ECN_DEMAND_CWR bit in ecn_flags, when really it
should let its caller, __tcp_ecn_check_ce() set TCP_ECN_DEMAND_CWR, in
which case the code would already properly force an immediate ACK.
So, what if we use this fix instead (not compiled, not tested):
diff --git a/net/ipv4/tcp_dctcp.c b/net/ipv4/tcp_dctcp.c
index 5f5e5936760e..4fecd2824edb 100644
--- a/net/ipv4/tcp_dctcp.c
+++ b/net/ipv4/tcp_dctcp.c
@@ -152,8 +152,6 @@ static void dctcp_ce_state_0_to_1(struct sock *sk)
ca->prior_rcv_nxt = tp->rcv_nxt;
ca->ce_state = 1;
-
- tp->ecn_flags |= TCP_ECN_DEMAND_CWR;
}
static void dctcp_ce_state_1_to_0(struct sock *sk)
What do you think?
neal
^ permalink raw reply related
* Re: [PATCH] [v2] infiniband: i40iw, nes: don't use wall time for TCP sequence numbers
From: kbuild test robot @ 2018-06-30 19:00 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild-all, Faisal Latif, Shiraz Saleem, Doug Ledford,
Jason Gunthorpe, David S. Miller, Arnd Bergmann, Henry Orosco,
Tatyana Nikolova, Mustafa Ismail, Bart Van Assche, Yuval Shaia,
linux-rdma, linux-kernel, netdev
In-Reply-To: <20180627132628.915978-1-arnd@arndb.de>
[-- Attachment #1: Type: text/plain, Size: 6089 bytes --]
Hi Arnd,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/infiniband-i40iw-nes-don-t-use-wall-time-for-TCP-sequence-numbers/20180627-221142
config: x86_64-randconfig-s2-06302231 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/infiniband/hw/i40iw/i40iw_cm.o: In function `i40iw_make_cm_node':
>> drivers/infiniband/hw/i40iw/i40iw_cm.c:2232: undefined reference to `secure_tcpv6_seq'
vim +2232 drivers/infiniband/hw/i40iw/i40iw_cm.c
2153
2154 /**
2155 * i40iw_make_cm_node - create a new instance of a cm node
2156 * @cm_core: cm's core
2157 * @iwdev: iwarp device structure
2158 * @cm_info: quad info for connection
2159 * @listener: passive connection's listener
2160 */
2161 static struct i40iw_cm_node *i40iw_make_cm_node(
2162 struct i40iw_cm_core *cm_core,
2163 struct i40iw_device *iwdev,
2164 struct i40iw_cm_info *cm_info,
2165 struct i40iw_cm_listener *listener)
2166 {
2167 struct i40iw_cm_node *cm_node;
2168 int oldarpindex;
2169 int arpindex;
2170 struct net_device *netdev = iwdev->netdev;
2171
2172 /* create an hte and cm_node for this instance */
2173 cm_node = kzalloc(sizeof(*cm_node), GFP_ATOMIC);
2174 if (!cm_node)
2175 return NULL;
2176
2177 /* set our node specific transport info */
2178 cm_node->ipv4 = cm_info->ipv4;
2179 cm_node->vlan_id = cm_info->vlan_id;
2180 if ((cm_node->vlan_id == I40IW_NO_VLAN) && iwdev->dcb)
2181 cm_node->vlan_id = 0;
2182 cm_node->tos = cm_info->tos;
2183 cm_node->user_pri = cm_info->user_pri;
2184 if (listener) {
2185 if (listener->tos != cm_info->tos)
2186 i40iw_debug(&iwdev->sc_dev, I40IW_DEBUG_DCB,
2187 "application TOS[%d] and remote client TOS[%d] mismatch\n",
2188 listener->tos, cm_info->tos);
2189 cm_node->tos = max(listener->tos, cm_info->tos);
2190 cm_node->user_pri = rt_tos2priority(cm_node->tos);
2191 i40iw_debug(&iwdev->sc_dev, I40IW_DEBUG_DCB, "listener: TOS:[%d] UP:[%d]\n",
2192 cm_node->tos, cm_node->user_pri);
2193 }
2194 memcpy(cm_node->loc_addr, cm_info->loc_addr, sizeof(cm_node->loc_addr));
2195 memcpy(cm_node->rem_addr, cm_info->rem_addr, sizeof(cm_node->rem_addr));
2196 cm_node->loc_port = cm_info->loc_port;
2197 cm_node->rem_port = cm_info->rem_port;
2198
2199 cm_node->mpa_frame_rev = iwdev->mpa_version;
2200 cm_node->send_rdma0_op = SEND_RDMA_READ_ZERO;
2201 cm_node->ird_size = I40IW_MAX_IRD_SIZE;
2202 cm_node->ord_size = I40IW_MAX_ORD_SIZE;
2203
2204 cm_node->listener = listener;
2205 cm_node->cm_id = cm_info->cm_id;
2206 ether_addr_copy(cm_node->loc_mac, netdev->dev_addr);
2207 spin_lock_init(&cm_node->retrans_list_lock);
2208 cm_node->ack_rcvd = false;
2209
2210 atomic_set(&cm_node->ref_count, 1);
2211 /* associate our parent CM core */
2212 cm_node->cm_core = cm_core;
2213 cm_node->tcp_cntxt.loc_id = I40IW_CM_DEF_LOCAL_ID;
2214 cm_node->tcp_cntxt.rcv_wscale = I40IW_CM_DEFAULT_RCV_WND_SCALE;
2215 cm_node->tcp_cntxt.rcv_wnd =
2216 I40IW_CM_DEFAULT_RCV_WND_SCALED >> I40IW_CM_DEFAULT_RCV_WND_SCALE;
2217 if (cm_node->ipv4) {
2218 cm_node->tcp_cntxt.loc_seq_num = secure_tcp_seq(htonl(cm_node->loc_addr[0]),
2219 htonl(cm_node->rem_addr[0]),
2220 htons(cm_node->loc_port),
2221 htons(cm_node->rem_port));
2222 cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV4;
2223 } else {
2224 __be32 loc[4] = {
2225 htonl(cm_node->loc_addr[0]), htonl(cm_node->loc_addr[1]),
2226 htonl(cm_node->loc_addr[2]), htonl(cm_node->loc_addr[3])
2227 };
2228 __be32 rem[4] = {
2229 htonl(cm_node->rem_addr[0]), htonl(cm_node->rem_addr[1]),
2230 htonl(cm_node->rem_addr[2]), htonl(cm_node->rem_addr[3])
2231 };
> 2232 cm_node->tcp_cntxt.loc_seq_num = secure_tcpv6_seq(loc, rem,
2233 htons(cm_node->loc_port),
2234 htons(cm_node->rem_port));
2235 cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV6;
2236 }
2237
2238 cm_node->iwdev = iwdev;
2239 cm_node->dev = &iwdev->sc_dev;
2240
2241 if ((cm_node->ipv4 &&
2242 i40iw_ipv4_is_loopback(cm_node->loc_addr[0], cm_node->rem_addr[0])) ||
2243 (!cm_node->ipv4 && i40iw_ipv6_is_loopback(cm_node->loc_addr,
2244 cm_node->rem_addr))) {
2245 arpindex = i40iw_arp_table(iwdev,
2246 cm_node->rem_addr,
2247 false,
2248 NULL,
2249 I40IW_ARP_RESOLVE);
2250 } else {
2251 oldarpindex = i40iw_arp_table(iwdev,
2252 cm_node->rem_addr,
2253 false,
2254 NULL,
2255 I40IW_ARP_RESOLVE);
2256 if (cm_node->ipv4)
2257 arpindex = i40iw_addr_resolve_neigh(iwdev,
2258 cm_info->loc_addr[0],
2259 cm_info->rem_addr[0],
2260 oldarpindex);
2261 else if (IS_ENABLED(CONFIG_IPV6))
2262 arpindex = i40iw_addr_resolve_neigh_ipv6(iwdev,
2263 cm_info->loc_addr,
2264 cm_info->rem_addr,
2265 oldarpindex);
2266 else
2267 arpindex = -EINVAL;
2268 }
2269 if (arpindex < 0) {
2270 i40iw_pr_err("cm_node arpindex\n");
2271 kfree(cm_node);
2272 return NULL;
2273 }
2274 ether_addr_copy(cm_node->rem_mac, iwdev->arp_table[arpindex].mac_addr);
2275 i40iw_add_hte_node(cm_core, cm_node);
2276 cm_core->stats_nodes_created++;
2277 return cm_node;
2278 }
2279
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31721 bytes --]
^ permalink raw reply
* Re: [net-next 01/12] net/mlx5e: Add UDP GSO support
From: Boris Pismenny @ 2018-06-30 19:40 UTC (permalink / raw)
To: Willem de Bruijn, Saeed Mahameed
Cc: David Miller, Network Development, ogerlitz, yossiku,
Alexander Duyck
In-Reply-To: <ccec1043-2223-b9d8-e73e-3e20987c634d@mellanox.com>
On 06/30/18 19:06, Boris Pismenny wrote:
>
>
> On 06/30/18 01:19, Willem de Bruijn wrote:
>> On Fri, Jun 29, 2018 at 2:24 AM Saeed Mahameed <saeedm@mellanox.com>
>> wrote:
>>> From: Boris Pismenny <borisp@mellanox.com>
>>>
>>> This patch enables UDP GSO support. We enable this by using two WQEs
>>> the first is a UDP LSO WQE for all segments with equal length, and the
>>> second is for the last segment in case it has different length.
>>> Due to HW limitation, before sending, we must adjust the packet
>>> length fields.
>>>
>>> We measure performance between two Intel(R) Xeon(R) CPU E5-2643 v2
>>> @3.50GHz
>>> machines connected back-to-back with Connectx4-Lx (40Gbps) NICs.
>>> We compare single stream UDP, UDP GSO and UDP GSO with offload.
>>> Performance:
>>> | MSS (bytes) | Throughput (Gbps) | CPU
>>> utilization (%)
>>> UDP GSO offload | 1472 | 35.6 | 8%
>>> UDP GSO | 1472 | 25.5 | 17%
>>> UDP | 1472 | 10.2 | 17%
>>> UDP GSO offload | 1024 | 35.6 | 8%
>>> UDP GSO | 1024 | 19.2 | 17%
>>> UDP | 1024 | 5.7 | 17%
>>> UDP GSO offload | 512 | 33.8 | 16%
>>> UDP GSO | 512 | 10.4 | 17%
>>> UDP | 512 | 3.5 | 17%
>> Very nice results :)
> Thanks. We expect to have 100Gbps results by netdev0x12.
>
>>> +static void mlx5e_udp_gso_prepare_last_skb(struct sk_buff *skb,
>>> + struct sk_buff *nskb,
>>> + int remaining)
>>> +{
>>> + int bytes_needed = remaining, remaining_headlen,
>>> remaining_page_offset;
>>> + int headlen = skb_transport_offset(skb) + sizeof(struct
>>> udphdr);
>>> + int payload_len = remaining + sizeof(struct udphdr);
>>> + int k = 0, i, j;
>>> +
>>> + skb_copy_bits(skb, 0, nskb->data, headlen);
>>> + nskb->dev = skb->dev;
>>> + skb_reset_mac_header(nskb);
>>> + skb_set_network_header(nskb, skb_network_offset(skb));
>>> + skb_set_transport_header(nskb, skb_transport_offset(skb));
>>> + skb_set_tail_pointer(nskb, headlen);
>>> +
>>> + /* How many frags do we need? */
>>> + for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
>>> + bytes_needed -=
>>> skb_frag_size(&skb_shinfo(skb)->frags[i]);
>>> + k++;
>>> + if (bytes_needed <= 0)
>>> + break;
>>> + }
>>> +
>>> + /* Fill the first frag and split it if necessary */
>>> + j = skb_shinfo(skb)->nr_frags - k;
>>> + remaining_page_offset = -bytes_needed;
>>> + skb_fill_page_desc(nskb, 0,
>>> + skb_shinfo(skb)->frags[j].page.p,
>>> + skb_shinfo(skb)->frags[j].page_offset + remaining_page_offset,
>>> + skb_shinfo(skb)->frags[j].size -
>>> remaining_page_offset);
>>> +
>>> + skb_frag_ref(skb, j);
>>> +
>>> + /* Fill the rest of the frags */
>>> + for (i = 1; i < k; i++) {
>>> + j = skb_shinfo(skb)->nr_frags - k + i;
>>> +
>>> + skb_fill_page_desc(nskb, i,
>>> + skb_shinfo(skb)->frags[j].page.p,
>>> + skb_shinfo(skb)->frags[j].page_offset,
>>> + skb_shinfo(skb)->frags[j].size);
>>> + skb_frag_ref(skb, j);
>>> + }
>>> + skb_shinfo(nskb)->nr_frags = k;
>>> +
>>> + remaining_headlen = remaining - skb->data_len;
>>> +
>>> + /* headlen contains remaining data? */
>>> + if (remaining_headlen > 0)
>>> + skb_copy_bits(skb, skb->len - remaining, nskb->data
>>> + headlen,
>>> + remaining_headlen);
>>> + nskb->len = remaining + headlen;
>>> + nskb->data_len = payload_len - sizeof(struct udphdr) +
>>> + max_t(int, 0, remaining_headlen);
>>> + nskb->protocol = skb->protocol;
>>> + if (nskb->protocol == htons(ETH_P_IP)) {
>>> + ip_hdr(nskb)->id = htons(ntohs(ip_hdr(nskb)->id) +
>>> + skb_shinfo(skb)->gso_segs);
>>> + ip_hdr(nskb)->tot_len =
>>> + htons(payload_len + sizeof(struct iphdr));
>>> + } else {
>>> + ipv6_hdr(nskb)->payload_len = htons(payload_len);
>>> + }
>>> + udp_hdr(nskb)->len = htons(payload_len);
>>> + skb_shinfo(nskb)->gso_size = 0;
>>> + nskb->ip_summed = skb->ip_summed;
>>> + nskb->csum_start = skb->csum_start;
>>> + nskb->csum_offset = skb->csum_offset;
>>> + nskb->queue_mapping = skb->queue_mapping;
>>> +}
>>> +
>>> +/* might send skbs and update wqe and pi */
>>> +struct sk_buff *mlx5e_udp_gso_handle_tx_skb(struct net_device *netdev,
>>> + struct mlx5e_txqsq *sq,
>>> + struct sk_buff *skb,
>>> + struct mlx5e_tx_wqe **wqe,
>>> + u16 *pi)
>>> +{
>>> + int payload_len = skb_shinfo(skb)->gso_size + sizeof(struct
>>> udphdr);
>>> + int headlen = skb_transport_offset(skb) + sizeof(struct
>>> udphdr);
>>> + int remaining = (skb->len - headlen) %
>>> skb_shinfo(skb)->gso_size;
>>> + struct sk_buff *nskb;
>>> +
>>> + if (skb->protocol == htons(ETH_P_IP))
>>> + ip_hdr(skb)->tot_len = htons(payload_len +
>>> sizeof(struct iphdr));
>>> + else
>>> + ipv6_hdr(skb)->payload_len = htons(payload_len);
>>> + udp_hdr(skb)->len = htons(payload_len);
>>> + if (!remaining)
>>> + return skb;
>>> +
>>> + nskb = alloc_skb(max_t(int, headlen, headlen + remaining -
>>> skb->data_len), GFP_ATOMIC);
>>> + if (unlikely(!nskb)) {
>>> + sq->stats->dropped++;
>>> + return NULL;
>>> + }
>>> +
>>> + mlx5e_udp_gso_prepare_last_skb(skb, nskb, remaining);
>>> +
>>> + skb_shinfo(skb)->gso_segs--;
>>> + pskb_trim(skb, skb->len - remaining);
>>> + mlx5e_sq_xmit(sq, skb, *wqe, *pi);
>>> + mlx5e_sq_fetch_wqe(sq, wqe, pi);
>>> + return nskb;
>>> +}
>> The device driver seems to be implementing the packet split here
>> similar to NETIF_F_GSO_PARTIAL. When advertising the right flag, the
>> stack should be able to do that for you and pass two packets to the
>> driver.
>
> We've totally missed that!
> Thank you. I'll fix and resubmit.
I've noticed that we could get cleaner code in our driver if we remove
these two lines from net/ipv4/udp_offload.c:
if (skb_is_gso(segs))
mss *= skb_shinfo(segs)->gso_segs;
I think that this is correct in case of GSO_PARTIAL segmentation for the
following reasons:
1. After this change the UDP payload field is consistent with the IP
header payload length field. Currently, IPv4 length is 1500 and UDP
total length is the full unsegmented length.
2. AFAIU, in GSO_PARTIAL no tunnel headers should be modified except the
IP ID field, including the UDP length field.
What do you think?
^ permalink raw reply
* Re: Anyone know if strongswan works with vrf?
From: David Ahern @ 2018-06-30 20:24 UTC (permalink / raw)
To: Ben Greear, netdev
In-Reply-To: <95b83150-a6dc-f43b-f605-bc7c7f5986e8@candelatech.com>
On 6/29/18 4:10 PM, Ben Greear wrote:
> Hello,
>
> We're trying to create lots of strongswan VPN tunnels on network devices
> bound to different VRFs. We are using Fedora-24 on the client side,
> with a 4.16.15+ kernel
> and updated 'ip' package, etc.
>
> So far, no luck getting it to work.
>
> Any idea if this is supported or not?
Kernel side xfrm code does work; been a couple of years since I tried
it. As I recall strongswan needs an update.
Looking at the 'ip xfrm' based scripts, you need to add 'sel dev ${VRF}'
to the state. eg.,
VRF="sel dev blue"
ip xfrm state add src ${REMIP} dst ${MYIP} \
proto esp spi 0x02122b77 reqid 0 mode tunnel \
replay-window 4 replay-oseq 0x4 \
auth-trunc 'hmac(md5)' 0xd94fcfea65fddf21dc6e0d24a0253508 96 \
enc 'cbc(des3_ede)' 0xfc46c20f8048be9725930ff3fb07ac2a91f0347dffeacf62 \
${VRF}
^ 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