* Re: [PATCH net-next v6 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-24 11:46 UTC (permalink / raw)
To: Daniel Borkmann, Simon Horman
Cc: davem, netdev, jiri, paulb, john.fastabend, mrv, hadarh, ogerlitz,
roid, xiyou.wangcong
In-Reply-To: <58862CE7.2080906@iogearbox.net>
On 17-01-23 11:18 AM, Daniel Borkmann wrote:
> On 01/23/2017 01:58 PM, Simon Horman wrote:
>> Hi Jamal,
>>
>> On Sun, Jan 22, 2017 at 03:25:50PM -0500, Jamal Hadi Salim wrote:
>>
>> ...
>>
>>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>> index cd08df9..58cf1c5 100644
>>> --- a/net/sched/act_api.c
>>> +++ b/net/sched/act_api.c
>>> @@ -24,6 +24,7 @@
>>> #include <net/net_namespace.h>
>>> #include <net/sock.h>
>>> #include <net/sch_generic.h>
>>> +#include <net/pkt_cls.h>
>>> #include <net/act_api.h>
>>> #include <net/netlink.h>
>>>
>>> @@ -33,6 +34,8 @@ static void free_tcf(struct rcu_head *head)
>>>
>>> free_percpu(p->cpu_bstats);
>>> free_percpu(p->cpu_qstats);
>>> + kfree(p->act_cookie->data);
>>
>> Does the above need to be protected by a check for p->act_cookie being
>> non-NULL?
>
> Yep, that would be a NULL-deref. Why not just embedd tc_cookie as
> suggested earlier, the struct is rather small anyway ...
>
Everytime I make a change like that i seem to forget to run one
more test and it creates more bugs (example, the last two resends
are errors introduced by changing the struct from last one which
was your suggestion;->).
So you will have to forgive me I am not going back to that
definition; I will post version 7 soon.
cheers,
jamal
^ permalink raw reply
* Re: [patch net-next v2 1/4] net: Introduce psample, a new genetlink channel for packet sampling
From: Simon Horman @ 2017-01-24 11:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, yotamg, idosch, eladr, nogahf, ogerlitz, jhs,
geert+renesas, stephen, xiyou.wangcong, linux, roopa,
john.fastabend, mrv
In-Reply-To: <20170124083734.GB1891@nanopsycho>
On Tue, Jan 24, 2017 at 09:37:34AM +0100, Jiri Pirko wrote:
> Tue, Jan 24, 2017 at 09:32:05AM CET, simon.horman@netronome.com wrote:
> >On Mon, Jan 23, 2017 at 11:07:08AM +0100, Jiri Pirko wrote:
> >> From: Yotam Gigi <yotamg@mellanox.com>
> >>
> >> Add a general way for kernel modules to sample packets, without being tied
> >> to any specific subsystem. This netlink channel can be used by tc,
> >> iptables, etc. and allow to standardize packet sampling in the kernel.
> >>
> >> For every sampled packet, the psample module adds the following metadata
> >> fields:
> >>
> >> PSAMPLE_ATTR_IIFINDEX - the packets input ifindex, if applicable
> >>
> >> PSAMPLE_ATTR_OIFINDEX - the packet output ifindex, if applicable
> >>
> >> PSAMPLE_ATTR_ORIGSIZE - the packet's original size, in case it has been
> >> truncated during sampling
> >>
> >> PSAMPLE_ATTR_SAMPLE_GROUP - the packet's sample group, which is set by the
> >> user who initiated the sampling. This field allows the user to
> >> differentiate between several samplers working simultaneously and
> >> filter packets relevant to him
> >>
> >> PSAMPLE_ATTR_GROUP_SEQ - sequence counter of last sent packet. The
> >> sequence is kept for each group
> >>
> >> PSAMPLE_ATTR_SAMPLE_RATE - the sampling rate used for sampling the packets
> >>
> >> PSAMPLE_ATTR_DATA - the actual packet bits
> >>
> >> The sampled packets are sent to the PSAMPLE_NL_MCGRP_SAMPLE multicast
> >> group. In addition, add the GET_GROUPS netlink command which allows the
> >> user to see the current sample groups, their refcount and sequence number.
> >> This command currently supports only netlink dump mode.
> >>
> >> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> >> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> >
> >Hi Jiri, Hi Yotam,
> >
> >this looks good to me.
> >
> >Reviewed-by: Simon Horman <simon.horman@netronome.com>
> >
> >I do, however, have one question: what is your feeling about allowing
> >the per-action cooking which Jamal has proposed[1] to be emited as
> >metadata as another PSAMPLE_ATTR_* attribute? For one thing I think it
> >would allow for smooth integration with OvS user-space which makes use of a
> >cookie.
>
> Yeah, cookie could be easily added.
Great. We can address that as follow-up work at some point.
^ permalink raw reply
* Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
From: Simon Horman @ 2017-01-24 11:49 UTC (permalink / raw)
To: Yotam Gigi
Cc: Jiri Pirko, netdev@vger.kernel.org, davem@davemloft.net,
Ido Schimmel, Elad Raz, Nogah Frankel, Or Gerlitz,
jhs@mojatatu.com, geert+renesas@glider.be,
stephen@networkplumber.org, xiyou.wangcong@gmail.com,
linux@roeck-us.net, roopa@cumulusnetworks.com,
john.fastabend@gmail.com, mrv@mojatatu.com
In-Reply-To: <DB3PR05MB076447766A61C7CD63F5875CAC750@DB3PR05MB0764.eurprd05.prod.outlook.com>
On Tue, Jan 24, 2017 at 08:39:37AM +0000, Yotam Gigi wrote:
> >-----Original Message-----
> >From: Simon Horman [mailto:simon.horman@netronome.com]
> >Sent: Tuesday, January 24, 2017 10:33 AM
> >To: Jiri Pirko <jiri@resnulli.us>
> >Cc: netdev@vger.kernel.org; davem@davemloft.net; Yotam Gigi
> ><yotamg@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> ><eladr@mellanox.com>; Nogah Frankel <nogahf@mellanox.com>; Or Gerlitz
> ><ogerlitz@mellanox.com>; jhs@mojatatu.com; geert+renesas@glider.be;
> >stephen@networkplumber.org; xiyou.wangcong@gmail.com; linux@roeck-us.net;
> >roopa@cumulusnetworks.com; john.fastabend@gmail.com; mrv@mojatatu.com
> >Subject: Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
> >
> >On Mon, Jan 23, 2017 at 11:07:09AM +0100, Jiri Pirko wrote:
> >> From: Yotam Gigi <yotamg@mellanox.com>
> >>
> >> This action allows the user to sample traffic matched by tc classifier.
> >> The sampling consists of choosing packets randomly and sampling them using
> >> the psample module. The user can configure the psample group number, the
> >> sampling rate and the packet's truncation (to save kernel-user traffic).
> >>
> >> Example:
> >> To sample ingress traffic from interface eth1, one may use the commands:
> >>
> >> tc qdisc add dev eth1 handle ffff: ingress
> >>
> >> tc filter add dev eth1 parent ffff: \
> >> matchall action sample rate 12 group 4
> >>
> >> Where the first command adds an ingress qdisc and the second starts
> >> sampling randomly with an average of one sampled packet per 12 packets on
> >> dev eth1 to psample group 4.
> >>
> >> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> >> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> >
> >Reviewed-by: Simon Horman <simon.horman@netronome.com>
> >
> >Is the tc user-space (iproute2) code available yet?
>
> Yes it is. I thought about sending it the moment the kernel patches gets
> accepted.
>
> Do you want it to send it directly to you?
I'm happy to wait for now.
^ permalink raw reply
* Re: [PATCH for bnxt_re V4 15/21] RDMA/bnxt_re: Support post_recv
From: Leon Romanovsky @ 2017-01-24 11:59 UTC (permalink / raw)
To: Selvin Xavier
Cc: dledford, linux-rdma, netdev, michael.chan, Eddie Wai,
Devesh Sharma, Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <1482320530-5344-16-git-send-email-selvin.xavier@broadcom.com>
[-- Attachment #1: Type: text/plain, Size: 3989 bytes --]
On Wed, Dec 21, 2016 at 03:42:04AM -0800, Selvin Xavier wrote:
> Enables the fastpath verb ib_post_recv.
>
> v3: Fixes sparse warnings
>
> Signed-off-by: Eddie Wai <eddie.wai@broadcom.com>
> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 123 +++++++++++++++++++++++++++++++
> drivers/infiniband/hw/bnxt_re/ib_verbs.h | 2 +
> drivers/infiniband/hw/bnxt_re/main.c | 2 +
> drivers/infiniband/hw/bnxt_re/qplib_fp.c | 100 +++++++++++++++++++++++++
> drivers/infiniband/hw/bnxt_re/qplib_fp.h | 8 ++
> 5 files changed, 235 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index e659490..7476994c 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -1637,6 +1637,51 @@ static int bnxt_re_build_qp1_send_v2(struct bnxt_re_qp *qp,
> return rc;
> }
>
> +/* For the MAD layer, it only provides the recv SGE the size of
> + * ib_grh + MAD datagram. No Ethernet headers, Ethertype, BTH, DETH,
> + * nor RoCE iCRC. The Cu+ solution must provide buffer for the entire
> + * receive packet (334 bytes) with no VLAN and then copy the GRH
> + * and the MAD datagram out to the provided SGE.
> + */
> +static int bnxt_re_build_qp1_shadow_qp_recv(struct bnxt_re_qp *qp,
> + struct ib_recv_wr *wr,
> + struct bnxt_qplib_swqe *wqe,
> + int payload_size)
> +{
> + struct bnxt_qplib_sge ref, sge;
> + u32 rq_prod_index;
> + struct bnxt_re_sqp_entries *sqp_entry;
> +
> + rq_prod_index = bnxt_qplib_get_rq_prod_index(&qp->qplib_qp);
> +
> + if (bnxt_qplib_get_qp1_rq_buf(&qp->qplib_qp, &sge)) {
> + /* Create 1 SGE to receive the entire
> + * ethernet packet
> + */
> + /* Save the reference from ULP */
> + ref.addr = wqe->sg_list[0].addr;
> + ref.lkey = wqe->sg_list[0].lkey;
> + ref.size = wqe->sg_list[0].size;
> +
> + sqp_entry = &qp->rdev->sqp_tbl[rq_prod_index];
> +
> + /* SGE 1 */
> + wqe->sg_list[0].addr = sge.addr;
> + wqe->sg_list[0].lkey = sge.lkey;
> + wqe->sg_list[0].size = BNXT_QPLIB_MAX_QP1_RQ_HDR_SIZE_V2;
> + sge.size -= wqe->sg_list[0].size;
> +
> + sqp_entry->sge.addr = ref.addr;
> + sqp_entry->sge.lkey = ref.lkey;
> + sqp_entry->sge.size = ref.size;
> + /* Store the wrid for reporting completion */
> + sqp_entry->wrid = wqe->wr_id;
> + /* change the wqe->wrid to table index */
> + wqe->wr_id = rq_prod_index;
> + }
> + return 0;
> +}
> +
> static int is_ud_qp(struct bnxt_re_qp *qp)
> {
> return qp->qplib_qp.type == CMDQ_CREATE_QP_TYPE_UD;
> @@ -1983,6 +2028,84 @@ int bnxt_re_post_send(struct ib_qp *ib_qp, struct ib_send_wr *wr,
> return rc;
> }
>
> +static int bnxt_re_post_recv_shadow_qp(struct bnxt_re_dev *rdev,
> + struct bnxt_re_qp *qp,
> + struct ib_recv_wr *wr)
> +{
> + struct bnxt_qplib_swqe wqe;
> + int rc = 0, payload_sz = 0;
> +
> + memset(&wqe, 0, sizeof(wqe));
> + while (wr) {
> + /* House keeping */
> + memset(&wqe, 0, sizeof(wqe));
> +
> + /* Common */
> + wqe.num_sge = wr->num_sge;
> + if (wr->num_sge > qp->qplib_qp.rq.max_sge) {
> + dev_err(rdev_to_dev(rdev),
> + "Limit exceeded for Receive SGEs");
> + rc = -EINVAL;
> + goto bad;
"goto bad" directly to other place in while() to call the "break".
It will be more convenient to call "break" here.
> + }
> + payload_sz = bnxt_re_build_sgl(wr->sg_list, wqe.sg_list,
> + wr->num_sge);
> + wqe.wr_id = wr->wr_id;
> + wqe.type = BNXT_QPLIB_SWQE_TYPE_RECV;
> +
> + if (!rc)
If we are here, we will always have rc == 0.
> + rc = bnxt_qplib_post_recv(&qp->qplib_qp, &wqe);
> +bad:
> + if (rc)
> + break;
> +
> + wr = wr->next;
> + }
> + bnxt_qplib_post_recv_db(&qp->qplib_qp);
> + return rc;
> +}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH net v1 3/6] tipc: fix connection refcount error
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
Until now, the generic server framework maintains the connection
id's per subscriber in server's conn_idr. At tipc_close_conn, we
remove the connection id from the server list, but the connection is
valid until we call the refcount cleanup. Hence we have a window
where the server allocates the same connection to an new subscriber
leading to inconsistent reference count. We have another refcount
warning we grab the refcount in tipc_conn_lookup() for connections
with flag with CF_CONNECTED not set. This usually occurs at shutdown
when the we stop the topology server and withdraw TIPC_CFG_SRV
publication thereby triggering a withdraw message to subscribers.
In this commit, we:
1. remove the connection from the server list at recount cleanup.
2. grab the refcount for a connection only if CF_CONNECTED is set.
Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 215849ce453d..2e803601aa99 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -91,7 +91,8 @@ static void tipc_sock_release(struct tipc_conn *con);
static void tipc_conn_kref_release(struct kref *kref)
{
struct tipc_conn *con = container_of(kref, struct tipc_conn, kref);
- struct sockaddr_tipc *saddr = con->server->saddr;
+ struct tipc_server *s = con->server;
+ struct sockaddr_tipc *saddr = s->saddr;
struct socket *sock = con->sock;
struct sock *sk;
@@ -106,6 +107,11 @@ static void tipc_conn_kref_release(struct kref *kref)
tipc_sock_release(con);
sock_release(sock);
con->sock = NULL;
+
+ spin_lock_bh(&s->idr_lock);
+ idr_remove(&s->conn_idr, con->conid);
+ s->idr_in_use--;
+ spin_unlock_bh(&s->idr_lock);
}
tipc_clean_outqueues(con);
@@ -128,8 +134,10 @@ static struct tipc_conn *tipc_conn_lookup(struct tipc_server *s, int conid)
spin_lock_bh(&s->idr_lock);
con = idr_find(&s->conn_idr, conid);
- if (con)
+ if (con && test_bit(CF_CONNECTED, &con->flags))
conn_get(con);
+ else
+ con = NULL;
spin_unlock_bh(&s->idr_lock);
return con;
}
@@ -198,15 +206,8 @@ static void tipc_sock_release(struct tipc_conn *con)
static void tipc_close_conn(struct tipc_conn *con)
{
- struct tipc_server *s = con->server;
-
if (test_and_clear_bit(CF_CONNECTED, &con->flags)) {
- spin_lock_bh(&s->idr_lock);
- idr_remove(&s->conn_idr, con->conid);
- s->idr_in_use--;
- spin_unlock_bh(&s->idr_lock);
-
/* We shouldn't flush pending works as we may be in the
* thread. In fact the races with pending rx/tx work structs
* are harmless for us here as we have already deleted this
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net v1 4/6] tipc: fix nametbl_lock soft lockup at module exit
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
Commit 333f796235a527 ("tipc: fix a race condition leading to
subscriber refcnt bug") reveals a soft lockup while acquiring
nametbl_lock.
Before commit 333f796235a527, we call tipc_conn_shutdown() from
tipc_close_conn() in the context of tipc_topsrv_stop(). In that
context, we are allowed to grab the nametbl_lock.
Commit 333f796235a527, moved tipc_conn_release (renamed from
tipc_conn_shutdown) to the connection refcount cleanup. This allows
either tipc_nametbl_withdraw() or tipc_topsrv_stop() to the cleanup.
Since tipc_exit_net() first calls tipc_topsrv_stop() and then
tipc_nametble_withdraw() increases the chances for the later to
perform the connection cleanup.
The soft lockup occurs in the call chain of tipc_nametbl_withdraw(),
when it performs the tipc_conn_kref_release() as it tries to grab
nametbl_lock again while holding it already.
tipc_nametbl_withdraw() grabs nametbl_lock
tipc_nametbl_remove_publ()
tipc_subscrp_report_overlap()
tipc_subscrp_send_event()
tipc_conn_sendmsg()
<< if (con->flags != CF_CONNECTED) we do conn_put(),
triggering the cleanup as refcount=0. >>
tipc_conn_kref_release
tipc_sock_release
tipc_conn_release
tipc_subscrb_delete
tipc_subscrp_delete
tipc_nametbl_unsubscribe << Soft Lockup >>
The previous changes in this series fixes the race conditions fixed
by commit 333f796235a527. Hence we can now revert the commit.
Fixes: 333f796235a52727 ("tipc: fix a race condition leading to subscriber refcnt bug")
Reported-and-Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 2e803601aa99..826cde2c401e 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -86,7 +86,6 @@ struct outqueue_entry {
static void tipc_recv_work(struct work_struct *work);
static void tipc_send_work(struct work_struct *work);
static void tipc_clean_outqueues(struct tipc_conn *con);
-static void tipc_sock_release(struct tipc_conn *con);
static void tipc_conn_kref_release(struct kref *kref)
{
@@ -104,7 +103,6 @@ static void tipc_conn_kref_release(struct kref *kref)
}
saddr->scope = -TIPC_NODE_SCOPE;
kernel_bind(sock, (struct sockaddr *)saddr, sizeof(*saddr));
- tipc_sock_release(con);
sock_release(sock);
con->sock = NULL;
@@ -194,19 +192,15 @@ static void tipc_unregister_callbacks(struct tipc_conn *con)
write_unlock_bh(&sk->sk_callback_lock);
}
-static void tipc_sock_release(struct tipc_conn *con)
+static void tipc_close_conn(struct tipc_conn *con)
{
struct tipc_server *s = con->server;
- if (con->conid)
- s->tipc_conn_release(con->conid, con->usr_data);
-
- tipc_unregister_callbacks(con);
-}
-
-static void tipc_close_conn(struct tipc_conn *con)
-{
if (test_and_clear_bit(CF_CONNECTED, &con->flags)) {
+ tipc_unregister_callbacks(con);
+
+ if (con->conid)
+ s->tipc_conn_release(con->conid, con->usr_data);
/* We shouldn't flush pending works as we may be in the
* thread. In fact the races with pending rx/tx work structs
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net v1 5/6] tipc: ignore requests when the connection state is not CONNECTED
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
In tipc_conn_sendmsg(), we first queue the request to the outqueue
followed by the connection state check. If the connection is not
connected, we should not queue this message.
In this commit, we reject the messages if the connection state is
not CF_CONNECTED.
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Tested-by: John Thompson <thompa.atl@gmail.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 826cde2c401e..04ff441b8065 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -453,6 +453,11 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
if (!con)
return -EINVAL;
+ if (!test_bit(CF_CONNECTED, &con->flags)) {
+ conn_put(con);
+ return 0;
+ }
+
e = tipc_alloc_entry(data, len);
if (!e) {
conn_put(con);
@@ -466,12 +471,8 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
list_add_tail(&e->list, &con->outqueue);
spin_unlock_bh(&con->outqueue_lock);
- if (test_bit(CF_CONNECTED, &con->flags)) {
- if (!queue_work(s->send_wq, &con->swork))
- conn_put(con);
- } else {
+ if (!queue_work(s->send_wq, &con->swork))
conn_put(con);
- }
return 0;
}
@@ -495,7 +496,7 @@ static void tipc_send_to_sock(struct tipc_conn *con)
int ret;
spin_lock_bh(&con->outqueue_lock);
- while (1) {
+ while (test_bit(CF_CONNECTED, &con->flags)) {
e = list_entry(con->outqueue.next, struct outqueue_entry,
list);
if ((struct list_head *) e == &con->outqueue)
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net v1 6/6] tipc: fix cleanup at module unload
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
In tipc_server_stop(), we iterate over the connections with limiting
factor as server's idr_in_use. We ignore the fact that this variable
is decremented in tipc_close_conn(), leading to premature exit.
In this commit, we iterate until the we have no connections left.
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Tested-by: John Thompson <thompa.atl@gmail.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 04ff441b8065..3cd6402e812c 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -619,14 +619,12 @@ int tipc_server_start(struct tipc_server *s)
void tipc_server_stop(struct tipc_server *s)
{
struct tipc_conn *con;
- int total = 0;
int id;
spin_lock_bh(&s->idr_lock);
- for (id = 0; total < s->idr_in_use; id++) {
+ for (id = 0; s->idr_in_use; id++) {
con = idr_find(&s->conn_idr, id);
if (con) {
- total++;
spin_unlock_bh(&s->idr_lock);
tipc_close_conn(con);
spin_lock_bh(&s->idr_lock);
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net v1 0/6] topology server fixes for nametable soft lockup
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: tipc-discussion, jon.maloy, ying.xue
In this series, we revert the commit 333f796235a527 ("tipc: fix a
race condition leading to subscriber refcnt bug") and provide an
alternate solution to fix the race conditions in commits 2-4.
We have to do this as the above commit introduced a nametbl soft
lockup at module exit as described by patch#4.
Parthasarathy Bhuvaragan (6):
tipc: fix nametbl_lock soft lockup at node/link events
tipc: add subscription refcount to avoid invalid delete
tipc: fix connection refcount error
tipc: fix nametbl_lock soft lockup at module exit
tipc: ignore requests when the connection state is not CONNECTED
tipc: fix cleanup at module unload
net/tipc/node.c | 9 +++-
net/tipc/server.c | 48 +++++++++------------
net/tipc/subscr.c | 124 ++++++++++++++++++++++++++++++------------------------
net/tipc/subscr.h | 1 +
4 files changed, 99 insertions(+), 83 deletions(-)
--
2.1.4
^ permalink raw reply
* [PATCH net v1 2/6] tipc: add subscription refcount to avoid invalid delete
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: tipc-discussion, jon.maloy, ying.xue
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
Until now, the subscribers keep track of the subscriptions using
reference count at subscriber level. At subscription cancel or
subscriber delete, we delete the subscription only if the timer
was pending for the subscription. This approach is incorrect as:
1. del_timer() is not SMP safe, if on CPU0 the check for pending
timer returns true but CPU1 might schedule the timer callback
thereby deleting the subscription. Thus when CPU0 is scheduled,
it deletes an invalid subscription.
2. We export tipc_subscrp_report_overlap(), which accesses the
subscription pointer multiple times. Meanwhile the subscription
timer can expire thereby freeing the subscription and we might
continue to access the subscription pointer leading to memory
violations.
In this commit, we introduce subscription refcount to avoid deleting
an invalid subscription.
Reported-and-Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/subscr.c | 124 ++++++++++++++++++++++++++++++------------------------
net/tipc/subscr.h | 1 +
2 files changed, 71 insertions(+), 54 deletions(-)
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 0dd02244e21d..9d94e65d0894 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -54,6 +54,8 @@ struct tipc_subscriber {
static void tipc_subscrp_delete(struct tipc_subscription *sub);
static void tipc_subscrb_put(struct tipc_subscriber *subscriber);
+static void tipc_subscrp_put(struct tipc_subscription *subscription);
+static void tipc_subscrp_get(struct tipc_subscription *subscription);
/**
* htohl - convert value to endianness used by destination
@@ -123,6 +125,7 @@ void tipc_subscrp_report_overlap(struct tipc_subscription *sub, u32 found_lower,
{
struct tipc_name_seq seq;
+ tipc_subscrp_get(sub);
tipc_subscrp_convert_seq(&sub->evt.s.seq, sub->swap, &seq);
if (!tipc_subscrp_check_overlap(&seq, found_lower, found_upper))
return;
@@ -132,30 +135,23 @@ void tipc_subscrp_report_overlap(struct tipc_subscription *sub, u32 found_lower,
tipc_subscrp_send_event(sub, found_lower, found_upper, event, port_ref,
node);
+ tipc_subscrp_put(sub);
}
static void tipc_subscrp_timeout(unsigned long data)
{
struct tipc_subscription *sub = (struct tipc_subscription *)data;
- struct tipc_subscriber *subscriber = sub->subscriber;
/* Notify subscriber of timeout */
tipc_subscrp_send_event(sub, sub->evt.s.seq.lower, sub->evt.s.seq.upper,
TIPC_SUBSCR_TIMEOUT, 0, 0);
- spin_lock_bh(&subscriber->lock);
- tipc_subscrp_delete(sub);
- spin_unlock_bh(&subscriber->lock);
-
- tipc_subscrb_put(subscriber);
+ tipc_subscrp_put(sub);
}
static void tipc_subscrb_kref_release(struct kref *kref)
{
- struct tipc_subscriber *subcriber = container_of(kref,
- struct tipc_subscriber, kref);
-
- kfree(subcriber);
+ kfree(container_of(kref,struct tipc_subscriber, kref));
}
static void tipc_subscrb_put(struct tipc_subscriber *subscriber)
@@ -168,6 +164,59 @@ static void tipc_subscrb_get(struct tipc_subscriber *subscriber)
kref_get(&subscriber->kref);
}
+static void tipc_subscrp_kref_release(struct kref *kref)
+{
+ struct tipc_subscription *sub = container_of(kref,
+ struct tipc_subscription,
+ kref);
+ struct tipc_net *tn = net_generic(sub->net, tipc_net_id);
+ struct tipc_subscriber *subscriber = sub->subscriber;
+
+ spin_lock_bh(&subscriber->lock);
+ tipc_nametbl_unsubscribe(sub);
+ list_del(&sub->subscrp_list);
+ atomic_dec(&tn->subscription_count);
+ spin_unlock_bh(&subscriber->lock);
+ kfree(sub);
+ tipc_subscrb_put(subscriber);
+}
+
+static void tipc_subscrp_put(struct tipc_subscription *subscription)
+{
+ kref_put(&subscription->kref, tipc_subscrp_kref_release);
+}
+
+static void tipc_subscrp_get(struct tipc_subscription *subscription)
+{
+ kref_get(&subscription->kref);
+}
+
+/* tipc_subscrb_subscrp_delete - delete a specific subscription or all
+ * subscriptions for a given subscriber.
+ */
+static void tipc_subscrb_subscrp_delete(struct tipc_subscriber *subscriber,
+ struct tipc_subscr *s)
+{
+ struct list_head *subscription_list = &subscriber->subscrp_list;
+ struct tipc_subscription *sub, *temp;
+
+ spin_lock_bh(&subscriber->lock);
+ list_for_each_entry_safe(sub, temp, subscription_list, subscrp_list) {
+ if (s && memcmp(s, &sub->evt.s, sizeof(struct tipc_subscr)))
+ continue;
+
+ tipc_subscrp_get(sub);
+ spin_unlock_bh(&subscriber->lock);
+ tipc_subscrp_delete(sub);
+ tipc_subscrp_put(sub);
+ spin_lock_bh(&subscriber->lock);
+
+ if (s)
+ break;
+ }
+ spin_unlock_bh(&subscriber->lock);
+}
+
static struct tipc_subscriber *tipc_subscrb_create(int conid)
{
struct tipc_subscriber *subscriber;
@@ -177,8 +226,8 @@ static struct tipc_subscriber *tipc_subscrb_create(int conid)
pr_warn("Subscriber rejected, no memory\n");
return NULL;
}
- kref_init(&subscriber->kref);
INIT_LIST_HEAD(&subscriber->subscrp_list);
+ kref_init(&subscriber->kref);
subscriber->conid = conid;
spin_lock_init(&subscriber->lock);
@@ -187,55 +236,22 @@ static struct tipc_subscriber *tipc_subscrb_create(int conid)
static void tipc_subscrb_delete(struct tipc_subscriber *subscriber)
{
- struct tipc_subscription *sub, *temp;
- u32 timeout;
-
- spin_lock_bh(&subscriber->lock);
- /* Destroy any existing subscriptions for subscriber */
- list_for_each_entry_safe(sub, temp, &subscriber->subscrp_list,
- subscrp_list) {
- timeout = htohl(sub->evt.s.timeout, sub->swap);
- if ((timeout == TIPC_WAIT_FOREVER) || del_timer(&sub->timer)) {
- tipc_subscrp_delete(sub);
- tipc_subscrb_put(subscriber);
- }
- }
- spin_unlock_bh(&subscriber->lock);
-
+ tipc_subscrb_subscrp_delete(subscriber, NULL);
tipc_subscrb_put(subscriber);
}
static void tipc_subscrp_delete(struct tipc_subscription *sub)
{
- struct tipc_net *tn = net_generic(sub->net, tipc_net_id);
+ u32 timeout = htohl(sub->evt.s.timeout, sub->swap);
- tipc_nametbl_unsubscribe(sub);
- list_del(&sub->subscrp_list);
- kfree(sub);
- atomic_dec(&tn->subscription_count);
+ if (timeout == TIPC_WAIT_FOREVER || del_timer(&sub->timer))
+ tipc_subscrp_put(sub);
}
static void tipc_subscrp_cancel(struct tipc_subscr *s,
struct tipc_subscriber *subscriber)
{
- struct tipc_subscription *sub, *temp;
- u32 timeout;
-
- spin_lock_bh(&subscriber->lock);
- /* Find first matching subscription, exit if not found */
- list_for_each_entry_safe(sub, temp, &subscriber->subscrp_list,
- subscrp_list) {
- if (!memcmp(s, &sub->evt.s, sizeof(struct tipc_subscr))) {
- timeout = htohl(sub->evt.s.timeout, sub->swap);
- if ((timeout == TIPC_WAIT_FOREVER) ||
- del_timer(&sub->timer)) {
- tipc_subscrp_delete(sub);
- tipc_subscrb_put(subscriber);
- }
- break;
- }
- }
- spin_unlock_bh(&subscriber->lock);
+ tipc_subscrb_subscrp_delete(subscriber, s);
}
static struct tipc_subscription *tipc_subscrp_create(struct net *net,
@@ -272,6 +288,7 @@ static struct tipc_subscription *tipc_subscrp_create(struct net *net,
sub->swap = swap;
memcpy(&sub->evt.s, s, sizeof(*s));
atomic_inc(&tn->subscription_count);
+ kref_init(&sub->kref);
return sub;
}
@@ -288,17 +305,16 @@ static void tipc_subscrp_subscribe(struct net *net, struct tipc_subscr *s,
spin_lock_bh(&subscriber->lock);
list_add(&sub->subscrp_list, &subscriber->subscrp_list);
- tipc_subscrb_get(subscriber);
sub->subscriber = subscriber;
tipc_nametbl_subscribe(sub);
+ tipc_subscrb_get(subscriber);
spin_unlock_bh(&subscriber->lock);
+ setup_timer(&sub->timer, tipc_subscrp_timeout, (unsigned long)sub);
timeout = htohl(sub->evt.s.timeout, swap);
- if (timeout == TIPC_WAIT_FOREVER)
- return;
- setup_timer(&sub->timer, tipc_subscrp_timeout, (unsigned long)sub);
- mod_timer(&sub->timer, jiffies + msecs_to_jiffies(timeout));
+ if (timeout != TIPC_WAIT_FOREVER)
+ mod_timer(&sub->timer, jiffies + msecs_to_jiffies(timeout));
}
/* Handle one termination request for the subscriber */
diff --git a/net/tipc/subscr.h b/net/tipc/subscr.h
index be60103082c9..ffdc214c117a 100644
--- a/net/tipc/subscr.h
+++ b/net/tipc/subscr.h
@@ -57,6 +57,7 @@ struct tipc_subscriber;
* @evt: template for events generated by subscription
*/
struct tipc_subscription {
+ struct kref kref;
struct tipc_subscriber *subscriber;
struct net *net;
struct timer_list timer;
--
2.1.4
^ permalink raw reply related
* [PATCH net v1 1/6] tipc: fix nametbl_lock soft lockup at node/link events
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: tipc-discussion, jon.maloy, ying.xue
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
We trigger a soft lockup as we grab nametbl_lock twice if the node
has a pending node up/down or link up/down event while:
- we process an incoming named message in tipc_named_rcv() and
perform an tipc_update_nametbl().
- we have pending backlog items in the name distributor queue
during a nametable update using tipc_nametbl_publish() or
tipc_nametbl_withdraw().
The following are the call chain associated:
tipc_named_rcv() Grabs nametbl_lock
tipc_update_nametbl() (publish/withdraw)
tipc_node_subscribe()/unsubscribe()
tipc_node_write_unlock()
<< lockup occurs if an outstanding node/link event
exits, as we grabs nametbl_lock again >>
tipc_nametbl_withdraw() Grab nametbl_lock
tipc_named_process_backlog()
tipc_update_nametbl()
<< rest as above >>
The function tipc_node_write_unlock(), in addition to releasing the
lock processes the outstanding node/link up/down events. To do this,
we need to grab the nametbl_lock again leading to the lockup.
In this commit we fix the soft lockup by introducing a fast variant of
node_unlock(), where we just release the lock. We adapt the
node_subscribe()/node_unsubscribe() to use the fast variants.
Reported-and-Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/node.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 9d2f4c2b08ab..27753325e06e 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -263,6 +263,11 @@ static void tipc_node_write_lock(struct tipc_node *n)
write_lock_bh(&n->lock);
}
+static void tipc_node_write_unlock_fast(struct tipc_node *n)
+{
+ write_unlock_bh(&n->lock);
+}
+
static void tipc_node_write_unlock(struct tipc_node *n)
{
struct net *net = n->net;
@@ -417,7 +422,7 @@ void tipc_node_subscribe(struct net *net, struct list_head *subscr, u32 addr)
}
tipc_node_write_lock(n);
list_add_tail(subscr, &n->publ_list);
- tipc_node_write_unlock(n);
+ tipc_node_write_unlock_fast(n);
tipc_node_put(n);
}
@@ -435,7 +440,7 @@ void tipc_node_unsubscribe(struct net *net, struct list_head *subscr, u32 addr)
}
tipc_node_write_lock(n);
list_del_init(subscr);
- tipc_node_write_unlock(n);
+ tipc_node_write_unlock_fast(n);
tipc_node_put(n);
}
--
2.1.4
^ permalink raw reply related
* [PATCH net-next v7 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-24 12:02 UTC (permalink / raw)
To: davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it. The user can store whatever they wish in the
128 bits.
Sample exercise(showing variable length use of cookie)
.. create an accept action with cookie a1b2c3d4
sudo $TC actions add action ok index 1 cookie a1b2c3d4
.. dump all gact actions..
sudo $TC -s actions ls action gact
action order 0: gact action pass
random type none pass val 0
index 1 ref 1 bind 0 installed 5 sec used 5 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie a1b2c3d4
.. bind the accept action to a filter..
sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
... send some traffic..
$ ping 127.0.0.1 -c 3
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
--- 127.0.0.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2109ms
rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
... show some stats
$ sudo $TC -s actions get action gact index 1
action order 1: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 204 sec used 5 sec
Action statistics:
Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie a1b2c3d4
.. try longer cookie...
$ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
.. dump..
$ sudo $TC -s actions ls action gact
action order 1: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 204 sec used 5 sec
Action statistics:
Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie 1234567890abcdef
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
Changes in v7:
-put guard around freeing cookie (caught by Simon)
-separate out the creation of the cookie (suggested by Simon)
Changes in v6:
- fix mem leak caught by Florian
Changes in V5:
- kill the stylistic changes
- Adopt a new structure with length-valuepointer representation
- rename some things
Changes in v4:
- move stylistic changes out into a separate patch
(and add more stylistic changes)
Changes in v3:
- use TC_ prefix for the max size
- move the cookie struct so visible only to kernel
- remove unneeded void * cast
Changes in V2:
-move from a union to a length-value representation
include/net/act_api.h | 1 +
include/net/pkt_cls.h | 8 ++++++++
include/uapi/linux/pkt_cls.h | 3 +++
net/sched/act_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 57 insertions(+)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1d71644..cfa2ae3 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -41,6 +41,7 @@ struct tc_action {
struct rcu_head tcfa_rcu;
struct gnet_stats_basic_cpu __percpu *cpu_bstats;
struct gnet_stats_queue __percpu *cpu_qstats;
+ struct tc_cookie *act_cookie;
};
#define tcf_head common.tcfa_head
#define tcf_index common.tcfa_index
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index f0a0514..b43077e 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -515,4 +515,12 @@ struct tc_cls_bpf_offload {
u32 gen_flags;
};
+
+/* This structure holds cookie structure that is passed from user
+ * to the kernel for actions and classifiers
+ */
+struct tc_cookie {
+ u8 *data;
+ u32 len;
+};
#endif
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index fd373eb..345551e 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -4,6 +4,8 @@
#include <linux/types.h>
#include <linux/pkt_sched.h>
+#define TC_COOKIE_MAX_SIZE 16
+
/* Action attributes */
enum {
TCA_ACT_UNSPEC,
@@ -12,6 +14,7 @@ enum {
TCA_ACT_INDEX,
TCA_ACT_STATS,
TCA_ACT_PAD,
+ TCA_ACT_COOKIE,
__TCA_ACT_MAX
};
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index cd08df9..3c5e29b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -24,6 +24,7 @@
#include <net/net_namespace.h>
#include <net/sock.h>
#include <net/sch_generic.h>
+#include <net/pkt_cls.h>
#include <net/act_api.h>
#include <net/netlink.h>
@@ -33,6 +34,12 @@ static void free_tcf(struct rcu_head *head)
free_percpu(p->cpu_bstats);
free_percpu(p->cpu_qstats);
+
+ if (p->act_cookie) {
+ kfree(p->act_cookie->data);
+ kfree(p->act_cookie);
+ }
+
kfree(p);
}
@@ -475,6 +482,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
goto nla_put_failure;
if (tcf_action_copy_stats(skb, a, 0))
goto nla_put_failure;
+ if (a->act_cookie) {
+ if (nla_put(skb, TCA_ACT_COOKIE, a->act_cookie->len,
+ a->act_cookie->data))
+ goto nla_put_failure;
+ }
+
nest = nla_nest_start(skb, TCA_OPTIONS);
if (nest == NULL)
goto nla_put_failure;
@@ -516,6 +529,22 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
return err;
}
+int nla_memdup_cookie(struct tc_action *a, struct nlattr **tb)
+{
+ a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
+ if (!a->act_cookie)
+ return -ENOMEM;
+
+ a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
+ if (!a->act_cookie->data) {
+ kfree(a->act_cookie);
+ return -ENOMEM;
+ }
+ a->act_cookie->len = nla_len(tb[TCA_ACT_COOKIE]);
+
+ return 0;
+}
+
struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
struct nlattr *est, char *name, int ovr,
int bind)
@@ -575,6 +604,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
if (err < 0)
goto err_mod;
+ if (tb[TCA_ACT_COOKIE]) {
+ int cklen = nla_len(tb[TCA_ACT_COOKIE]);
+
+ if (cklen > TC_COOKIE_MAX_SIZE) {
+ err = -EINVAL;
+ tcf_hash_release(a, bind);
+ goto err_mod;
+ }
+
+ err = nla_memdup_cookie(a, tb);
+ if (err < 0) {
+ tcf_hash_release(a, bind);
+ goto err_mod;
+ }
+ }
+
/* module count goes up only when brand new policy is created
* if it exists and is only bound to in a_o->init() then
* ACT_P_CREATED is not returned (a zero is).
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 2/3] ath10k: use dma_zalloc_coherent()
From: Valo, Kalle @ 2017-01-24 12:13 UTC (permalink / raw)
To: Joe Perches
Cc: Srinivas Kandagatla, ath10k@lists.infradead.org,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1485235529.12563.39.camel@perches.com>
Joe Perches <joe@perches.com> writes:
> On Tue, 2017-01-24 at 05:18 +0000, Valo, Kalle wrote:
>> Joe Perches <joe@perches.com> writes:
>>
>> > On Mon, 2017-01-23 at 15:04 +0000, Srinivas Kandagatla wrote:
>> > > use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().
>> >
>> > []
>> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c
>> > > b/drivers/net/wireless/ath/ath10k/pci.c
>> >
>> > []
>> > > @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
>> > > */
>> > > alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
>> > >
>> > > - data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
>> > > + data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
>> > > alloc_nbytes,
>> > > &ce_data_base,
>> > > GFP_ATOMIC);
>> >
>> > trivia:
>> >
>> > Nicer to realign arguments and remove the unnecessary cast.
>> >
>> > Perhaps:
>> >
>> > data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, &ce_data_base,
>> > GFP_ATOMIC);
>>
>> Sure, but that should be in a separate patch.
>
> I don't think so, trivial patches can be combined.
>
> It's also nicer to realign all modified multiline
> arguments when performing these changes.
>
> Coccinelle generally does it automatically.
A matter of preference really. I prefer keeping style and functional
changes in separate patches, keeps the review simple. And style changes
can hide bugs.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH net-next v7 1/1] net sched actions: Add support for user cookies
From: Jiri Pirko @ 2017-01-24 12:15 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: davem, netdev, jiri, paulb, john.fastabend, simon.horman, mrv,
hadarh, ogerlitz, roid, xiyou.wangcong, daniel
In-Reply-To: <1485259361-16860-1-git-send-email-jhs@emojatatu.com>
Tue, Jan 24, 2017 at 01:02:41PM CET, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>Introduce optional 128-bit action cookie.
>Like all other cookie schemes in the networking world (eg in protocols
>like http or existing kernel fib protocol field, etc) the idea is to save
>user state that when retrieved serves as a correlator. The kernel
>_should not_ intepret it. The user can store whatever they wish in the
>128 bits.
>
>Sample exercise(showing variable length use of cookie)
>
>.. create an accept action with cookie a1b2c3d4
>sudo $TC actions add action ok index 1 cookie a1b2c3d4
>
>.. dump all gact actions..
>sudo $TC -s actions ls action gact
>
> action order 0: gact action pass
> random type none pass val 0
> index 1 ref 1 bind 0 installed 5 sec used 5 sec
> Action statistics:
> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie a1b2c3d4
>
>.. bind the accept action to a filter..
>sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
>u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
>
>... send some traffic..
>$ ping 127.0.0.1 -c 3
>PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
>64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
>64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
>64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
>
>--- 127.0.0.1 ping statistics ---
>3 packets transmitted, 3 received, 0% packet loss, time 2109ms
>rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
>
>... show some stats
>$ sudo $TC -s actions get action gact index 1
>
> action order 1: gact action pass
> random type none pass val 0
> index 1 ref 2 bind 1 installed 204 sec used 5 sec
> Action statistics:
> Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie a1b2c3d4
>
>.. try longer cookie...
>$ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
>.. dump..
>$ sudo $TC -s actions ls action gact
>
> action order 1: gact action pass
> random type none pass val 0
> index 1 ref 2 bind 1 installed 204 sec used 5 sec
> Action statistics:
> Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie 1234567890abcdef
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
>Changes in v7:
> -put guard around freeing cookie (caught by Simon)
> -separate out the creation of the cookie (suggested by Simon)
>
>Changes in v6:
> - fix mem leak caught by Florian
>
>Changes in V5:
> - kill the stylistic changes
> - Adopt a new structure with length-valuepointer representation
> - rename some things
>
>Changes in v4:
> - move stylistic changes out into a separate patch
> (and add more stylistic changes)
>
>Changes in v3:
> - use TC_ prefix for the max size
> - move the cookie struct so visible only to kernel
> - remove unneeded void * cast
>
>Changes in V2:
> -move from a union to a length-value representation
>
> include/net/act_api.h | 1 +
> include/net/pkt_cls.h | 8 ++++++++
> include/uapi/linux/pkt_cls.h | 3 +++
> net/sched/act_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 57 insertions(+)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index 1d71644..cfa2ae3 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -41,6 +41,7 @@ struct tc_action {
> struct rcu_head tcfa_rcu;
> struct gnet_stats_basic_cpu __percpu *cpu_bstats;
> struct gnet_stats_queue __percpu *cpu_qstats;
>+ struct tc_cookie *act_cookie;
> };
> #define tcf_head common.tcfa_head
> #define tcf_index common.tcfa_index
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index f0a0514..b43077e 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -515,4 +515,12 @@ struct tc_cls_bpf_offload {
> u32 gen_flags;
> };
>
>+
>+/* This structure holds cookie structure that is passed from user
>+ * to the kernel for actions and classifiers
>+ */
>+struct tc_cookie {
>+ u8 *data;
>+ u32 len;
>+};
> #endif
>diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>index fd373eb..345551e 100644
>--- a/include/uapi/linux/pkt_cls.h
>+++ b/include/uapi/linux/pkt_cls.h
>@@ -4,6 +4,8 @@
> #include <linux/types.h>
> #include <linux/pkt_sched.h>
>
>+#define TC_COOKIE_MAX_SIZE 16
>+
> /* Action attributes */
> enum {
> TCA_ACT_UNSPEC,
>@@ -12,6 +14,7 @@ enum {
> TCA_ACT_INDEX,
> TCA_ACT_STATS,
> TCA_ACT_PAD,
>+ TCA_ACT_COOKIE,
> __TCA_ACT_MAX
> };
>
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index cd08df9..3c5e29b 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -24,6 +24,7 @@
> #include <net/net_namespace.h>
> #include <net/sock.h>
> #include <net/sch_generic.h>
>+#include <net/pkt_cls.h>
> #include <net/act_api.h>
> #include <net/netlink.h>
>
>@@ -33,6 +34,12 @@ static void free_tcf(struct rcu_head *head)
>
> free_percpu(p->cpu_bstats);
> free_percpu(p->cpu_qstats);
>+
>+ if (p->act_cookie) {
>+ kfree(p->act_cookie->data);
>+ kfree(p->act_cookie);
>+ }
>+
> kfree(p);
> }
>
>@@ -475,6 +482,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> goto nla_put_failure;
> if (tcf_action_copy_stats(skb, a, 0))
> goto nla_put_failure;
>+ if (a->act_cookie) {
>+ if (nla_put(skb, TCA_ACT_COOKIE, a->act_cookie->len,
>+ a->act_cookie->data))
>+ goto nla_put_failure;
>+ }
>+
> nest = nla_nest_start(skb, TCA_OPTIONS);
> if (nest == NULL)
> goto nla_put_failure;
>@@ -516,6 +529,22 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
> return err;
> }
>
>+int nla_memdup_cookie(struct tc_action *a, struct nlattr **tb)
>+{
>+ a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
>+ if (!a->act_cookie)
>+ return -ENOMEM;
>+
>+ a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
you can do just:
size_t len = nla_len(tb[TCA_ACT_COOKIE];
a->act_cookie = kzalloc(sizeof(*a->act_cookie) + len, GFP_KERNEL);
if (!a->act_cookie)
return -ENOMEM;
memcpy(a->act_cookie->data, nla_data(tb[TCA_ACT_COOKIE], len));
a->act_cookie->len = len;
return 0;
Really see no need to alloc 2 chunks instead of one. But as you like.
>+ if (!a->act_cookie->data) {
>+ kfree(a->act_cookie);
>+ return -ENOMEM;
>+ }
>+ a->act_cookie->len = nla_len(tb[TCA_ACT_COOKIE]);
>+
>+ return 0;
>+}
>+
> struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> struct nlattr *est, char *name, int ovr,
> int bind)
>@@ -575,6 +604,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> if (err < 0)
> goto err_mod;
>
>+ if (tb[TCA_ACT_COOKIE]) {
>+ int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>+
>+ if (cklen > TC_COOKIE_MAX_SIZE) {
>+ err = -EINVAL;
>+ tcf_hash_release(a, bind);
>+ goto err_mod;
>+ }
>+
>+ err = nla_memdup_cookie(a, tb);
>+ if (err < 0) {
You can do just "if (err)", but anyway:
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>+ tcf_hash_release(a, bind);
>+ goto err_mod;
>+ }
>+ }
>+
> /* module count goes up only when brand new policy is created
> * if it exists and is only bound to in a_o->init() then
> * ACT_P_CREATED is not returned (a zero is).
>--
>1.9.1
>
^ permalink raw reply
* Re: [PATCH for bnxt_re V4 17/21] RDMA/bnxt_re: Handling dispatching of events to IB stack
From: Leon Romanovsky @ 2017-01-24 12:18 UTC (permalink / raw)
To: Selvin Xavier
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
michael.chan-dY08KVG/lbpWk0Htik3J/w, Eddie Wai, Devesh Sharma,
Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <1482320530-5344-18-git-send-email-selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4310 bytes --]
On Wed, Dec 21, 2016 at 03:42:06AM -0800, Selvin Xavier wrote:
> This patch implements events dispatching to the IB stack
> based on NETDEV events received.
>
> v2: Removed cleanup of the resources during driver unload since
> we are calling unregister_netdevice_notifier first in the exit.
>
> v3: Fixes cocci warnings and some sparse warnings
>
> Signed-off-by: Eddie Wai <eddie.wai-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Selvin Xavier <selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
> drivers/infiniband/hw/bnxt_re/main.c | 65 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index ab0b35a..bd13414 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -729,6 +729,60 @@ static int bnxt_re_alloc_res(struct bnxt_re_dev *rdev)
> return rc;
> }
>
> +static void bnxt_re_dispatch_event(struct ib_device *ibdev, struct ib_qp *qp,
> + u8 port_num, enum ib_event_type event)
> +{
> + struct ib_event ib_event;
> +
> + ib_event.device = ibdev;
> + if (qp)
> + ib_event.element.qp = qp;
> + else
> + ib_event.element.port_num = port_num;
> + ib_event.event = event;
> + ib_dispatch_event(&ib_event);
> +}
> +
> +static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> + struct bnxt_re_qp *qp)
> +{
> + return (qp->ib_qp.qp_type == IB_QPT_GSI) || (qp == rdev->qp1_sqp);
> +}
> +
> +static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev, bool qp_wait)
> +{
> + int mask = IB_QP_STATE, qp_count, count = 1;
> + struct ib_qp_attr qp_attr;
> + struct bnxt_re_qp *qp;
> +
> + qp_attr.qp_state = IB_QPS_ERR;
> + mutex_lock(&rdev->qp_lock);
> + list_for_each_entry(qp, &rdev->qp_list, list) {
> + /* Modify the state of all QPs except QP1/Shadow QP */
> + if (!bnxt_re_is_qp1_or_shadow_qp(rdev, qp)) {
> + if (qp->qplib_qp.state !=
> + CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> + qp->qplib_qp.state !=
> + CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> + bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
> + 1, IB_EVENT_QP_FATAL);
> + bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> + NULL);
> + }
> + }
> + }
> +
> + mutex_unlock(&rdev->qp_lock);
> + if (qp_wait) {
All callers to this function in this patch set qp_wait to be false.
Do you have in following patches qp_wait == true?
I'm curious because of your msleep below.
> + /* Give the application some time to clean up */
> + do {
> + qp_count = atomic_read(&rdev->qp_count);
> + msleep(100);
> + } while ((qp_count != atomic_read(&rdev->qp_count)) &&
> + count--);
> + }
> +}
> +
> static void bnxt_re_ib_unreg(struct bnxt_re_dev *rdev, bool lock_wait)
> {
> int i, rc;
> @@ -888,6 +942,9 @@ static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
> }
> }
> set_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags);
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1, IB_EVENT_PORT_ACTIVE);
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1, IB_EVENT_GID_CHANGE);
> +
> return 0;
> free_sctx:
> bnxt_re_net_stats_ctx_free(rdev, rdev->qplib_ctx.stats.fw_id, true);
> @@ -967,10 +1024,18 @@ static void bnxt_re_task(struct work_struct *work)
> "Failed to register with IB: %#x", rc);
> break;
> case NETDEV_UP:
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1,
> + IB_EVENT_PORT_ACTIVE);
> break;
> case NETDEV_DOWN:
> + bnxt_re_dev_stop(rdev, false);
> break;
> case NETDEV_CHANGE:
> + if (!netif_carrier_ok(rdev->netdev))
> + bnxt_re_dev_stop(rdev, false);
> + else if (netif_carrier_ok(rdev->netdev))
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1,
> + IB_EVENT_PORT_ACTIVE);
> break;
> default:
> break;
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH for bnxt_re V4 20/21] RDMA/bnxt_re: Add QP event handling
From: Leon Romanovsky @ 2017-01-24 12:20 UTC (permalink / raw)
To: Selvin Xavier
Cc: dledford, linux-rdma, netdev, michael.chan, Eddie Wai,
Devesh Sharma, Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <1482320530-5344-21-git-send-email-selvin.xavier@broadcom.com>
[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]
On Wed, Dec 21, 2016 at 03:42:09AM -0800, Selvin Xavier wrote:
> Implements callback handler for processing Async events related to a QP.
> This patch also implements the control path command completion handling.
>
> v3: Removes unwanted braces
>
> Signed-off-by: Eddie Wai <eddie.wai@broadcom.com>
> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
> drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 47 ++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> index 9144b5a..a000397 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> @@ -257,6 +257,44 @@ static int bnxt_qplib_process_func_event(struct bnxt_qplib_rcfw *rcfw,
> return 0;
> }
>
> +static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
> + struct creq_qp_event *qp_event)
> +{
> + struct bnxt_qplib_crsq *crsq = &rcfw->crsq;
> + struct bnxt_qplib_hwq *cmdq = &rcfw->cmdq;
> + struct bnxt_qplib_crsqe *crsqe;
> + u16 cbit, cookie, blocked = 0;
> + unsigned long flags;
> + u32 sw_cons;
> +
> + switch (qp_event->event) {
> + case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION:
> + break;
it looks like if( ... ) return 0;
> + default:
> + /* Command Response */
> + spin_lock_irqsave(&cmdq->lock, flags);
> + sw_cons = HWQ_CMP(crsq->cons, crsq);
> + crsqe = &crsq->crsq[sw_cons];
> + crsq->cons++;
> + memcpy(&crsqe->qp_event, qp_event, sizeof(crsqe->qp_event));
> +
> + cookie = le16_to_cpu(crsqe->qp_event.cookie);
> + blocked = cookie & RCFW_CMD_IS_BLOCKING;
> + cookie &= RCFW_MAX_COOKIE_VALUE;
> + cbit = cookie % RCFW_MAX_OUTSTANDING_CMD;
> + if (!test_and_clear_bit(cbit, rcfw->cmdq_bitmap))
> + dev_warn(&rcfw->pdev->dev,
> + "QPLIB: CMD bit %d was not requested", cbit);
> +
> + cmdq->cons += crsqe->req_size;
> + spin_unlock_irqrestore(&cmdq->lock, flags);
> + if (!blocked)
> + wake_up(&rcfw->waitq);
> + break;
> + }
> + return 0;
> +}
> +
> /* SP - CREQ Completion handlers */
> static void bnxt_qplib_service_creq(unsigned long data)
> {
> @@ -280,6 +318,15 @@ static void bnxt_qplib_service_creq(unsigned long data)
> type = creqe->type & CREQ_BASE_TYPE_MASK;
> switch (type) {
> case CREQ_BASE_TYPE_QP_EVENT:
> + if (!bnxt_qplib_process_qp_event
> + (rcfw, (struct creq_qp_event *)creqe))
> + rcfw->creq_qp_event_processed++;
> + else {
> + dev_warn(&rcfw->pdev->dev, "QPLIB: crsqe with");
> + dev_warn(&rcfw->pdev->dev,
> + "QPLIB: type = 0x%x not handled",
> + type);
> + }
> break;
> case CREQ_BASE_TYPE_FUNC_EVENT:
> if (!bnxt_qplib_process_func_event
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH][V3][net-next] net: sctp: fix array overrun read on sctp_timer_tbl
From: Neil Horman @ 2017-01-24 12:22 UTC (permalink / raw)
To: Colin King
Cc: Vlad Yasevich, David S . Miller, linux-sctp, netdev, linux-kernel
In-Reply-To: <20170124092554.27019-1-colin.king@canonical.com>
On Tue, Jan 24, 2017 at 09:25:54AM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Table sctp_timer_tbl is missing a TIMEOUT_RECONF string so
> add this in. Also compare timeout with the size of the array
> sctp_timer_tbl rather than SCTP_EVENT_TIMEOUT_MAX. Also add
> a build time check that SCTP_EVENT_TIMEOUT_MAX is correct
> so we don't ever get this kind of mismatch between the table
> and SCTP_EVENT_TIMEOUT_MAX in the future.
>
> Kudos to Marcelo Ricardo Leitner for spotting the missing string
> and suggesting the build time sanity check.
>
> Fixes CoverityScan CID#1397639 ("Out-of-bounds read")
>
> Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/sctp/debug.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/debug.c b/net/sctp/debug.c
> index 95d7b15..2e47eb2 100644
> --- a/net/sctp/debug.c
> +++ b/net/sctp/debug.c
> @@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
> "TIMEOUT_T4_RTO",
> "TIMEOUT_T5_SHUTDOWN_GUARD",
> "TIMEOUT_HEARTBEAT",
> + "TIMEOUT_RECONF",
> "TIMEOUT_SACK",
> "TIMEOUT_AUTOCLOSE",
> };
> @@ -166,7 +167,9 @@ static const char *const sctp_timer_tbl[] = {
> /* Lookup timer debug name. */
> const char *sctp_tname(const sctp_subtype_t id)
> {
> - if (id.timeout <= SCTP_EVENT_TIMEOUT_MAX)
> + BUILD_BUG_ON(SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl));
> +
> + if (id.timeout < ARRAY_SIZE(sctp_timer_tbl))
> return sctp_timer_tbl[id.timeout];
> return "unknown_timer";
> }
> --
> 2.10.2
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* RE: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: David Laight @ 2017-01-24 12:34 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner'
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
Neil Horman, Vlad Yasevich, davem@davemloft.net
In-Reply-To: <20170123184744.GC3781@localhost.localdomain>
From: Marcelo Ricardo Leitner
> Sent: 23 January 2017 18:48
> On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> > From: Xin Long
> > > Sent: 19 January 2017 17:19
> > > This patch is to implement Sender-Side Procedures for the Add
> > > Outgoing and Incoming Streams Request Parameter described in
> > > rfc6525 section 5.1.5-5.1.6.
> > >
> > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > 6.3.4 for users.
> > ...
> > > + out = params->sas_outstrms;
> > > + in = params->sas_instrms;
> > > +
> > > + if (!out && !in)
> > > + goto out;
> > > +
> > > + if (out) {
> > > + __u16 nums = stream->outcnt + out;
> >
> > Make nums 'unsigned int', the code will be smaller and you can
> > use the value for the overflow check.
>
> Smaller as in to avoid the sum below?
>
> >
> > > + /* Check for overflow, can't use nums here */
> > > + if (stream->outcnt + out > SCTP_MAX_STREAM)
> > > + goto out;
No, smaller as in not requiring the compiler to add instructions
to mask (or worse sign extend) the result of the arithmetic expression
to less than the number of bits in an 'int' when the result of the
expression is to be kept in a register.
The x86 is about the only modern cpu that has 8 and 16 bit arithmetic.
For everything else you really don't want to do arithmetic on char
and short unless you really want the wrapping to happen.
David
^ permalink raw reply
* RE: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: David Laight @ 2017-01-24 12:35 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner', Neil Horman
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
Vlad Yasevich, davem@davemloft.net
In-Reply-To: <20170123160234.GY3781@localhost.localdomain>
From: Marcelo Ricardo Leitner
> Sent: 23 January 2017 16:03
...
> > > Does kcalloc() zero the entire area, or just the length you ask for?
> > > If the latter you need to zero the rest here.
> > Better still, just use krealloc. You still need to zero out any space beyond
> > the old length, but it will make the code shorter, and avoid the need for
> > additional temporary variables.
>
> Seems if we pass gfp | __GFP_ZERO to krealloc it will end up zeroing the
> slab for us before doing the memcpy.
> I didn't follow all paths but in slab_alloc_node it will end up calling:
> if (unlikely(gfpflags & __GFP_ZERO) && object)
> memset(object, 0, s->object_size);
> So I would expect that other paths also do it.
You probably don't want krealloc() zeroing all of the new area.
David
^ permalink raw reply
* [PATCH net] net: free ip_vs_dest structs when refcnt=0
From: David Windsor @ 2017-01-24 12:48 UTC (permalink / raw)
To: netdev, kernel-hardening
Cc: keescook, elena.reshetova, ja, dwindsor, ishkamiel
Currently, the ip_vs_dest cache frees ip_vs_dest objects when their reference
count becomes < 0. Aside from not being semantically sound, this is problematic
for the new type refcount_t, which will be introduced shortly in a separate patch.
refcount_t is the new kernel type for holding reference counts, and provides
overflow protection and a constrained interface relative to atomic_t (the type
currently being used for kernel reference counts).
Per Juilan Anastasov: "The problem is that dest_trash currently holds deleted
dests (unlinked from RCU lists) with refcnt=0." Changing dest_trash to hold
dest with refcnt=1 will allow us to free ip_vs_dest structs when their refcnt=0,
in ip_vs_dest_put_and_free().
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
include/net/ip_vs.h | 2 +-
net/netfilter/ipvs/ip_vs_ctl.c | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index cd6018a..a3e78ad 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -1421,7 +1421,7 @@ static inline void ip_vs_dest_put(struct ip_vs_dest *dest)
static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
{
- if (atomic_dec_return(&dest->refcnt) < 0)
+ if (atomic_dec_and_test(&dest->refcnt))
kfree(dest);
}
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 55e0169..6b5492e 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -711,7 +711,6 @@ ip_vs_trash_get_dest(struct ip_vs_service *svc, int dest_af,
dest->vport == svc->port))) {
/* HIT */
list_del(&dest->t_list);
- ip_vs_dest_hold(dest);
goto out;
}
}
@@ -1084,7 +1083,6 @@ static void __ip_vs_del_dest(struct netns_ipvs *ipvs, struct ip_vs_dest *dest,
list_add(&dest->t_list, &ipvs->dest_trash);
dest->idle_start = 0;
spin_unlock_bh(&ipvs->dest_trash_lock);
- ip_vs_dest_put(dest);
}
@@ -1160,7 +1158,7 @@ static void ip_vs_dest_trash_expire(unsigned long data)
spin_lock(&ipvs->dest_trash_lock);
list_for_each_entry_safe(dest, next, &ipvs->dest_trash, t_list) {
- if (atomic_read(&dest->refcnt) > 0)
+ if (atomic_read(&dest->refcnt) > 1)
continue;
if (dest->idle_start) {
if (time_before(now, dest->idle_start +
--
2.7.4
^ permalink raw reply related
* Re: [patch net-next] sctp: fix some debug output
From: Marcelo Ricardo Leitner @ 2017-01-24 13:00 UTC (permalink / raw)
To: Dan Carpenter
Cc: Vlad Yasevich, Xin Long, Neil Horman, David S. Miller, linux-sctp,
netdev, kernel-janitors, Colin King
In-Reply-To: <20170124105007.GF4201@mwanda>
On Tue, Jan 24, 2017 at 01:50:07PM +0300, Dan Carpenter wrote:
> On Tue, Jan 24, 2017 at 07:14:11AM -0200, Marcelo Ricardo Leitner wrote:
> > On Tue, Jan 24, 2017 at 12:05:40PM +0300, Dan Carpenter wrote:
> > > We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
> > > it causes an off-by-one read overflow.
> > >
> > > Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > Weird, seems your patch is missing the --- marker here.
>
> Are there tools that require the --- marker? I normally leave it out
> when it's not required.
Yeah I don't think it's required. It's usually there to split it between
changelog and non-changelog stuff like the diffstat output, which is not
there too, so it should be okay.
Marcelo
^ permalink raw reply
* Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: 'Marcelo Ricardo Leitner' @ 2017-01-24 13:08 UTC (permalink / raw)
To: David Laight
Cc: Neil Horman, 'Xin Long', network dev,
linux-sctp@vger.kernel.org, Vlad Yasevich, davem@davemloft.net
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB026CAD7@AcuExch.aculab.com>
On Tue, Jan 24, 2017 at 12:35:39PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 23 January 2017 16:03
> ...
> > > > Does kcalloc() zero the entire area, or just the length you ask for?
> > > > If the latter you need to zero the rest here.
> > > Better still, just use krealloc. You still need to zero out any space beyond
> > > the old length, but it will make the code shorter, and avoid the need for
> > > additional temporary variables.
> >
> > Seems if we pass gfp | __GFP_ZERO to krealloc it will end up zeroing the
> > slab for us before doing the memcpy.
> > I didn't follow all paths but in slab_alloc_node it will end up calling:
> > if (unlikely(gfpflags & __GFP_ZERO) && object)
> > memset(object, 0, s->object_size);
> > So I would expect that other paths also do it.
>
> You probably don't want krealloc() zeroing all of the new area.
Yep, agreed.
Marcelo
^ permalink raw reply
* Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: 'Marcelo Ricardo Leitner' @ 2017-01-24 13:10 UTC (permalink / raw)
To: David Laight
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
Neil Horman, Vlad Yasevich, davem@davemloft.net
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB026CAC6@AcuExch.aculab.com>
On Tue, Jan 24, 2017 at 12:34:06PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 23 January 2017 18:48
> > On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> > > From: Xin Long
> > > > Sent: 19 January 2017 17:19
> > > > This patch is to implement Sender-Side Procedures for the Add
> > > > Outgoing and Incoming Streams Request Parameter described in
> > > > rfc6525 section 5.1.5-5.1.6.
> > > >
> > > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > > 6.3.4 for users.
> > > ...
> > > > + out = params->sas_outstrms;
> > > > + in = params->sas_instrms;
> > > > +
> > > > + if (!out && !in)
> > > > + goto out;
> > > > +
> > > > + if (out) {
> > > > + __u16 nums = stream->outcnt + out;
> > >
> > > Make nums 'unsigned int', the code will be smaller and you can
> > > use the value for the overflow check.
> >
> > Smaller as in to avoid the sum below?
> >
> > >
> > > > + /* Check for overflow, can't use nums here */
> > > > + if (stream->outcnt + out > SCTP_MAX_STREAM)
> > > > + goto out;
>
> No, smaller as in not requiring the compiler to add instructions
> to mask (or worse sign extend) the result of the arithmetic expression
> to less than the number of bits in an 'int' when the result of the
> expression is to be kept in a register.
>
> The x86 is about the only modern cpu that has 8 and 16 bit arithmetic.
> For everything else you really don't want to do arithmetic on char
> and short unless you really want the wrapping to happen.
Okay thanks.
Marcelo
^ permalink raw reply
* Re: [PATCHv5 net-next 0/5] sctp: add sender-side procedures for stream reconf asoc reset and add streams
From: Marcelo Ricardo Leitner @ 2017-01-24 13:11 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, Vlad Yasevich, davem
In-Reply-To: <20170123184217.GB3781@localhost.localdomain>
On Mon, Jan 23, 2017 at 04:42:17PM -0200, Marcelo Ricardo Leitner wrote:
> On Sat, Jan 21, 2017 at 06:24:21PM +0800, Xin Long wrote:
> > Patch 3/5 is to implement sender-side procedures for the SSN/TSN Reset
> > Request Parameter described in rfc6525 section 5.1.4, patch 2/5 is
> > ahead of it to define a function to make the request chunk for it.
> >
> > Patch 5/5 is to implement sender-side procedures for the Add Incoming
> > and Outgoing Streams Request Parameter Request Parameter described in
> > rfc6525 section 5.1.5 and 5.1.6, patch 4/5 is ahead of it to define a
> > function to make the request chunk for it.
> >
> > Patch 1/5 is a fix to recover streams states when it fails to send
> > request.
> >
> > v1->v2:
> > - put these into a smaller group.
> > - rename some temporary variables in the codes.
> > - rename the titles of the commits and improve some changelogs.
> > v2->v3:
> > - re-split the patchset and make sure it has no dead codes for review.
> > - move some codes into stream.c from socket.c.
> > v3->v4:
> > - add one more patch to fix a send reset stream request issue.
> > - doing actual work only when request is sent successfully.
> > - reduce some indents in sctp_send_add_streams.
> > v4->v5:
> > - close streams before sending request and recover them when sending
> > fails in patch 1/5 and patch 3/5
>
> To make it clear, the notes on v3 patchset also apply to this one, which
> are:
> - usage of krealloc()
> - the removal of __packed attribute
- the int change too.
>
> So there should be a v6 of it soon.
>
> Thanks
>
> >
> > Xin Long (5):
> > sctp: streams should be recovered when it fails to send request.
> > sctp: add support for generating stream reconf ssn/tsn reset request
> > chunk
> > sctp: implement sender-side procedures for SSN/TSN Reset Request
> > Parameter
> > sctp: add support for generating stream reconf add incoming/outgoing
> > streams request chunk
> > sctp: implement sender-side procedures for Add Incoming/Outgoing
> > Streams Request Parameter
> >
> > include/linux/sctp.h | 12 ++++
> > include/net/sctp/sctp.h | 3 +
> > include/net/sctp/sm.h | 5 ++
> > include/uapi/linux/sctp.h | 8 +++
> > net/sctp/sm_make_chunk.c | 75 +++++++++++++++++++++++++
> > net/sctp/socket.c | 58 +++++++++++++++++++
> > net/sctp/stream.c | 138 +++++++++++++++++++++++++++++++++++++++++++++-
> > 7 files changed, 298 insertions(+), 1 deletion(-)
> >
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] net: ethernet: mvneta: add support for 2.5G DRSGMII mode
From: Andrew Lunn @ 2017-01-24 13:19 UTC (permalink / raw)
To: Jan Lübbe
Cc: netdev, devicetree, davem, Rob Herring, Mark Rutland,
Thomas Petazzoni, Florian Fainelli
In-Reply-To: <1485246858.5508.47.camel@pengutronix.de>
> It works without any other changes. ;)
>
> Initially I looked at adding a SPEED_2500, but I wasn't sure which code
> would need to handle the new value. Would the path from the ethtool
> ioctl to the driver be enough (mvneta_ethtool_set_link_ksettings() as
> you said)?
You have this connected to an FPGA? Do you have a traditional PHY in
the FPGA? Or are you implementing something more like an Ethernet
switch?
I don't know this driver too well, but what often happens is the PHY
performs autoneg and the phylib then calls the adjust_link function to
set the MAC to what has been negotiated. So if you have a PHY attached
which can do 2.5Ghz, you might be seeing calls to adjust_link with
SPEED_2500.
What does ethtool <devname> show?
Thanks
Andrew
^ 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