| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20070910220841.GK11801@linux.vnet.ibm.com>
Date: Mon, 10 Sep 2007 15:08:41 -0700
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Vlad Yasevich <vladislav.yasevich@...com>
Cc: netdev@...r.kernel.org, lksctp-developers@...ts.sourceforge.net
Subject: Re: [RFC PATCH 2/2] SCTP: Convert bind_addr_list locking to RCU
On Mon, Sep 10, 2007 at 03:46:30PM -0400, Vlad Yasevich wrote:
> Since the sctp_sockaddr_entry is now RCU enabled as part of
> the patch to synchronize sctp_localaddr_list, it makes sense to
> change all handling of these entries to RCU. This includes the
> sctp_bind_addrs structure and it's list of bound addresses.
>
> This list is currently protected by an external rw_lock and that
> looks like an overkill. There are only 2 writers to the list:
> bind()/bindx() calls, and BH processing of ASCONF-ACK chunks.
> These are already seriealized via the socket lock, so they will
> not step on each other. These are also relatively rare, so we
> should be good with RCU.
>
> The readers are varied and they are easily converted to RCU.
Again, good start -- similar questions as for the other patch in this
series. Also a few places where it looks like you are letting a pointer
to an RCU-protected data structure slip out of rcu_read_lock() protection,
and a case of mixing rcu_read_lock() and rcu_read_lock_bh() within the
same RCU-protected data structure.
Thanx, Paul
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@...com>
> ---
> include/net/sctp/structs.h | 3 -
> net/sctp/associola.c | 14 +------
> net/sctp/bind_addr.c | 59 ++++++++++++++++++----------
> net/sctp/endpointola.c | 26 ++++---------
> net/sctp/ipv6.c | 12 ++---
> net/sctp/protocol.c | 25 +++++-------
> net/sctp/sm_make_chunk.c | 17 +++-----
> net/sctp/socket.c | 92 ++++++++++++++------------------------------
> 8 files changed, 97 insertions(+), 151 deletions(-)
>
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 2591c49..1d46f7d 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1222,9 +1222,6 @@ struct sctp_ep_common {
> * bind_addr.address_list is our set of local IP addresses.
> */
> struct sctp_bind_addr bind_addr;
> -
> - /* Protection during address list comparisons. */
> - rwlock_t addr_lock;
> };
>
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 2ad1caf..9bad8ba 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -99,7 +99,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>
> /* Initialize the bind addr area. */
> sctp_bind_addr_init(&asoc->base.bind_addr, ep->base.bind_addr.port);
> - rwlock_init(&asoc->base.addr_lock);
>
> asoc->state = SCTP_STATE_CLOSED;
>
> @@ -937,8 +936,6 @@ struct sctp_transport *sctp_assoc_is_match(struct sctp_association *asoc,
> {
> struct sctp_transport *transport;
>
> - sctp_read_lock(&asoc->base.addr_lock);
> -
> if ((htons(asoc->base.bind_addr.port) == laddr->v4.sin_port) &&
> (htons(asoc->peer.port) == paddr->v4.sin_port)) {
> transport = sctp_assoc_lookup_paddr(asoc, paddr);
> @@ -952,7 +949,6 @@ struct sctp_transport *sctp_assoc_is_match(struct sctp_association *asoc,
> transport = NULL;
>
> out:
> - sctp_read_unlock(&asoc->base.addr_lock);
> return transport;
> }
>
> @@ -1376,19 +1372,13 @@ int sctp_assoc_set_bind_addr_from_cookie(struct sctp_association *asoc,
> int sctp_assoc_lookup_laddr(struct sctp_association *asoc,
> const union sctp_addr *laddr)
> {
> - int found;
> + int found = 0;
>
> - sctp_read_lock(&asoc->base.addr_lock);
> if ((asoc->base.bind_addr.port == ntohs(laddr->v4.sin_port)) &&
> sctp_bind_addr_match(&asoc->base.bind_addr, laddr,
> - sctp_sk(asoc->base.sk))) {
> + sctp_sk(asoc->base.sk)))
> found = 1;
> - goto out;
> - }
>
> - found = 0;
> -out:
> - sctp_read_unlock(&asoc->base.addr_lock);
> return found;
> }
>
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index 7fc369f..9c7db1f 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -167,7 +167,10 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
>
> INIT_LIST_HEAD(&addr->list);
> INIT_RCU_HEAD(&addr->rcu);
> - list_add_tail(&addr->list, &bp->address_list);
> +
> + rcu_read_lock();
> + list_add_tail_rcu(&addr->list, &bp->address_list);
> + rcu_read_unlock();
Given the original code, we presumably hold the update-side lock. If so,
the rcu_read_lock() and rcu_read_unlock() are (harmlessly) redundant.
> SCTP_DBG_OBJCNT_INC(addr);
>
> return 0;
> @@ -178,20 +181,23 @@ int sctp_add_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *new,
> */
> int sctp_del_bind_addr(struct sctp_bind_addr *bp, union sctp_addr *del_addr)
> {
> - struct list_head *pos, *temp;
> - struct sctp_sockaddr_entry *addr;
> + struct sctp_sockaddr_entry *addr, *temp;
>
> - list_for_each_safe(pos, temp, &bp->address_list) {
> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
> + rcu_read_lock_bh();
> + list_for_each_entry_safe(addr, temp, &bp->address_list, list) {
> if (sctp_cmp_addr_exact(&addr->a, del_addr)) {
> /* Found the exact match. */
> - list_del(pos);
> - kfree(addr);
> - SCTP_DBG_OBJCNT_DEC(addr);
> -
> - return 0;
> + addr->valid = 0;
> + list_del_rcu(&addr->list);
> + break;
> }
> }
> + rcu_read_unlock_bh();
Ditto.
> +
> + if (addr && !addr->valid) {
> + call_rcu_bh(&addr->rcu, sctp_local_addr_free);
> + SCTP_DBG_OBJCNT_DEC(addr);
> + }
>
> return -EINVAL;
> }
> @@ -302,15 +308,20 @@ int sctp_bind_addr_match(struct sctp_bind_addr *bp,
> struct sctp_sock *opt)
> {
> struct sctp_sockaddr_entry *laddr;
> - struct list_head *pos;
> -
> - list_for_each(pos, &bp->address_list) {
> - laddr = list_entry(pos, struct sctp_sockaddr_entry, list);
> - if (opt->pf->cmp_addr(&laddr->a, addr, opt))
> - return 1;
> + int match = 0;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> + if (!laddr->valid)
> + continue;
As before, what happens if the entry is deleted by some other CPU at
this point, and thus ->valid is cleared? If harmless, why bother with
->valid?
> + if (opt->pf->cmp_addr(&laddr->a, addr, opt)) {
> + match = 1;
> + break;
> + }
> }
> + rcu_read_unlock();
>
> - return 0;
> + return match;
> }
>
> /* Find the first address in the bind address list that is not present in
> @@ -325,27 +336,31 @@ union sctp_addr *sctp_find_unmatch_addr(struct sctp_bind_addr *bp,
> union sctp_addr *addr;
> void *addr_buf;
> struct sctp_af *af;
> - struct list_head *pos;
> int i;
>
> - list_for_each(pos, &bp->address_list) {
> - laddr = list_entry(pos, struct sctp_sockaddr_entry, list);
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> + if (!laddr->valid)
> + continue;
Ditto...
>
> addr_buf = (union sctp_addr *)addrs;
> for (i = 0; i < addrcnt; i++) {
> addr = (union sctp_addr *)addr_buf;
> af = sctp_get_af_specific(addr->v4.sin_family);
> if (!af)
> - return NULL;
> + break;
>
> if (opt->pf->cmp_addr(&laddr->a, addr, opt))
> break;
>
> addr_buf += af->sockaddr_len;
> }
> - if (i == addrcnt)
> + if (i == addrcnt) {
> + rcu_read_unlock();
Since rcu_read_unlock() just happened, some other CPU is free to
free up this data structure. In a CONFIG_PREEMPT kernel (as well as a
CONFIG_PREEMPT_RT kernel, for that matter), this task might be preempted
at this point, and a full grace period might elapse.
In which case, the following statement returns a pointer to the freelist,
which is not good.
> return &laddr->a;
> + }
> }
> + rcu_read_unlock();
>
> return NULL;
> }
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index 1404a9e..fa10af5 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -92,7 +92,6 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
>
> /* Initialize the bind addr area */
> sctp_bind_addr_init(&ep->base.bind_addr, 0);
> - rwlock_init(&ep->base.addr_lock);
>
> /* Remember who we are attached to. */
> ep->base.sk = sk;
> @@ -225,21 +224,14 @@ void sctp_endpoint_put(struct sctp_endpoint *ep)
> struct sctp_endpoint *sctp_endpoint_is_match(struct sctp_endpoint *ep,
> const union sctp_addr *laddr)
> {
> - struct sctp_endpoint *retval;
> + struct sctp_endpoint *retval = NULL;
>
> - sctp_read_lock(&ep->base.addr_lock);
> if (htons(ep->base.bind_addr.port) == laddr->v4.sin_port) {
> if (sctp_bind_addr_match(&ep->base.bind_addr, laddr,
> - sctp_sk(ep->base.sk))) {
> + sctp_sk(ep->base.sk)))
> retval = ep;
> - goto out;
> - }
> }
>
> - retval = NULL;
> -
> -out:
> - sctp_read_unlock(&ep->base.addr_lock);
> return retval;
> }
>
> @@ -261,9 +253,7 @@ static struct sctp_association *__sctp_endpoint_lookup_assoc(
> list_for_each(pos, &ep->asocs) {
> asoc = list_entry(pos, struct sctp_association, asocs);
> if (rport == asoc->peer.port) {
> - sctp_read_lock(&asoc->base.addr_lock);
> *transport = sctp_assoc_lookup_paddr(asoc, paddr);
> - sctp_read_unlock(&asoc->base.addr_lock);
>
> if (*transport)
> return asoc;
> @@ -295,20 +285,20 @@ struct sctp_association *sctp_endpoint_lookup_assoc(
> int sctp_endpoint_is_peeled_off(struct sctp_endpoint *ep,
> const union sctp_addr *paddr)
> {
> - struct list_head *pos;
> struct sctp_sockaddr_entry *addr;
> struct sctp_bind_addr *bp;
>
> - sctp_read_lock(&ep->base.addr_lock);
> bp = &ep->base.bind_addr;
> - list_for_each(pos, &bp->address_list) {
> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
> + rcu_read_lock();
> + list_for_each_entry_rcu(addr, &bp->address_list, list) {
> + if (!addr->valid)
> + continue;
And ditto again...
> if (sctp_has_association(&addr->a, paddr)) {
> - sctp_read_unlock(&ep->base.addr_lock);
> + rcu_read_unlock();
> return 1;
> }
> }
> - sctp_read_unlock(&ep->base.addr_lock);
> + rcu_read_unlock();
>
> return 0;
> }
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index fc2e4e2..4f6dc55 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -302,9 +302,7 @@ static void sctp_v6_get_saddr(struct sctp_association *asoc,
> union sctp_addr *saddr)
> {
> struct sctp_bind_addr *bp;
> - rwlock_t *addr_lock;
> struct sctp_sockaddr_entry *laddr;
> - struct list_head *pos;
> sctp_scope_t scope;
> union sctp_addr *baddr = NULL;
> __u8 matchlen = 0;
> @@ -324,14 +322,14 @@ static void sctp_v6_get_saddr(struct sctp_association *asoc,
> scope = sctp_scope(daddr);
>
> bp = &asoc->base.bind_addr;
> - addr_lock = &asoc->base.addr_lock;
>
> /* Go through the bind address list and find the best source address
> * that matches the scope of the destination address.
> */
> - sctp_read_lock(addr_lock);
> - list_for_each(pos, &bp->address_list) {
> - laddr = list_entry(pos, struct sctp_sockaddr_entry, list);
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> + if (!laddr->valid)
> + continue;
Ditto yet again...
> if ((laddr->use_as_src) &&
> (laddr->a.sa.sa_family == AF_INET6) &&
> (scope <= sctp_scope(&laddr->a))) {
> @@ -353,7 +351,7 @@ static void sctp_v6_get_saddr(struct sctp_association *asoc,
> __FUNCTION__, asoc, NIP6(daddr->v6.sin6_addr));
> }
>
> - sctp_read_unlock(addr_lock);
> + rcu_read_unlock();
> }
>
> /* Make a copy of all potential local addresses. */
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index ac52f9e..a1030ed 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -222,7 +222,7 @@ int sctp_copy_local_addr_list(struct sctp_bind_addr *bp, sctp_scope_t scope,
> (copy_flags & SCTP_ADDR6_ALLOWED) &&
> (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
> error = sctp_add_bind_addr(bp, &addr->a, 1,
> - GFP_ATOMIC);
> + GFP_ATOMIC);
> if (error)
> goto end_copy;
> }
> @@ -426,9 +426,7 @@ static struct dst_entry *sctp_v4_get_dst(struct sctp_association *asoc,
> struct rtable *rt;
> struct flowi fl;
> struct sctp_bind_addr *bp;
> - rwlock_t *addr_lock;
> struct sctp_sockaddr_entry *laddr;
> - struct list_head *pos;
> struct dst_entry *dst = NULL;
> union sctp_addr dst_saddr;
>
> @@ -457,23 +455,20 @@ static struct dst_entry *sctp_v4_get_dst(struct sctp_association *asoc,
> goto out;
>
> bp = &asoc->base.bind_addr;
> - addr_lock = &asoc->base.addr_lock;
>
> if (dst) {
> /* Walk through the bind address list and look for a bind
> * address that matches the source address of the returned dst.
> */
> - sctp_read_lock(addr_lock);
> - list_for_each(pos, &bp->address_list) {
> - laddr = list_entry(pos, struct sctp_sockaddr_entry,
> - list);
> - if (!laddr->use_as_src)
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> + if (!laddr->valid || !laddr->use_as_src)
> continue;
And here as well...
> sctp_v4_dst_saddr(&dst_saddr, dst, htons(bp->port));
> if (sctp_v4_cmp_addr(&dst_saddr, &laddr->a))
> goto out_unlock;
> }
> - sctp_read_unlock(addr_lock);
> + rcu_read_unlock();
>
> /* None of the bound addresses match the source address of the
> * dst. So release it.
> @@ -485,10 +480,10 @@ static struct dst_entry *sctp_v4_get_dst(struct sctp_association *asoc,
> /* Walk through the bind address list and try to get a dst that
> * matches a bind address as the source address.
> */
> - sctp_read_lock(addr_lock);
> - list_for_each(pos, &bp->address_list) {
> - laddr = list_entry(pos, struct sctp_sockaddr_entry, list);
> -
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> + if (!laddr->valid)
> + continue;
OK, this is the last one I am flagging, you can find the others. ;-)
> if ((laddr->use_as_src) &&
> (AF_INET == laddr->a.sa.sa_family)) {
> fl.fl4_src = laddr->a.v4.sin_addr.s_addr;
> @@ -500,7 +495,7 @@ static struct dst_entry *sctp_v4_get_dst(struct sctp_association *asoc,
> }
>
> out_unlock:
> - sctp_read_unlock(addr_lock);
> + rcu_read_unlock();
> out:
> if (dst)
> SCTP_DEBUG_PRINTK("rt_dst:%u.%u.%u.%u, rt_src:%u.%u.%u.%u\n",
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 79856c9..caaa29f 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1531,7 +1531,7 @@ no_hmac:
> /* Also, add the destination address. */
> if (list_empty(&retval->base.bind_addr.address_list)) {
> sctp_add_bind_addr(&retval->base.bind_addr, &chunk->dest, 1,
> - GFP_ATOMIC);
> + GFP_ATOMIC);
> }
>
> retval->next_tsn = retval->c.initial_tsn;
> @@ -2613,22 +2613,17 @@ static int sctp_asconf_param_success(struct sctp_association *asoc,
>
> switch (asconf_param->param_hdr.type) {
> case SCTP_PARAM_ADD_IP:
> - sctp_local_bh_disable();
> - sctp_write_lock(&asoc->base.addr_lock);
> - list_for_each(pos, &bp->address_list) {
> - saddr = list_entry(pos, struct sctp_sockaddr_entry, list);
> + rcu_read_lock_bh();
> + list_for_each_entry_rcu(saddr, &bp->address_list, list) {
> + if (!saddr->valid)
> + continue;
> if (sctp_cmp_addr_exact(&saddr->a, &addr))
> saddr->use_as_src = 1;
> }
> - sctp_write_unlock(&asoc->base.addr_lock);
> - sctp_local_bh_enable();
> + rcu_read_unlock_bh();
If you use rcu_read_lock_bh() and rcu_read_unlock_bh() in one read path
for a given data structure, you need to use them in all the other read
paths for that data structure. In addition, you must use call_rcu_bh()
when deleting the corresponding data elements.
The normal and the _bh RCU grace periods are unrelated, so mixing them
for a given RCU-protected data structure is a bad idea. (Or are these
somehow two independent data structures?)
> break;
> case SCTP_PARAM_DEL_IP:
> - sctp_local_bh_disable();
> - sctp_write_lock(&asoc->base.addr_lock);
> retval = sctp_del_bind_addr(bp, &addr);
> - sctp_write_unlock(&asoc->base.addr_lock);
> - sctp_local_bh_enable();
> list_for_each(pos, &asoc->peer.transport_addr_list) {
> transport = list_entry(pos, struct sctp_transport,
> transports);
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index a3acf78..35cc30c 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -367,14 +367,10 @@ SCTP_STATIC int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
> if (!bp->port)
> bp->port = inet_sk(sk)->num;
>
> - /* Add the address to the bind address list. */
> - sctp_local_bh_disable();
> - sctp_write_lock(&ep->base.addr_lock);
> -
> - /* Use GFP_ATOMIC since BHs are disabled. */
> + /* Add the address to the bind address list.
> + * Use GFP_ATOMIC since BHs will be disabled.
> + */
> ret = sctp_add_bind_addr(bp, addr, 1, GFP_ATOMIC);
> - sctp_write_unlock(&ep->base.addr_lock);
> - sctp_local_bh_enable();
>
> /* Copy back into socket for getsockname() use. */
> if (!ret) {
> @@ -497,7 +493,6 @@ static int sctp_send_asconf_add_ip(struct sock *sk,
> void *addr_buf;
> struct sctp_af *af;
> struct list_head *pos;
> - struct list_head *p;
> int i;
> int retval = 0;
>
> @@ -544,14 +539,15 @@ static int sctp_send_asconf_add_ip(struct sock *sk,
> if (i < addrcnt)
> continue;
>
> - /* Use the first address in bind addr list of association as
> - * Address Parameter of ASCONF CHUNK.
> + /* Use the first valid address in bind addr list of
> + * association as Address Parameter of ASCONF CHUNK.
> */
> - sctp_read_lock(&asoc->base.addr_lock);
> bp = &asoc->base.bind_addr;
> - p = bp->address_list.next;
> - laddr = list_entry(p, struct sctp_sockaddr_entry, list);
> - sctp_read_unlock(&asoc->base.addr_lock);
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list)
> + if (laddr->valid)
> + break;
> + rcu_read_unlock();
Here you are carrying an RCU-protected data item (*laddr) outside of an
rcu_read_lock()/rcu_read_unlock() pair. This is not good -- you need
to move the rcu_read_unlock() farther down to cover the full extend to
uses of the laddr pointer.
Again, RCU is within its rights allowing a grace period to elapse, so
that past this point, laddr might well point into the freelist.
>
> chunk = sctp_make_asconf_update_ip(asoc, &laddr->a, addrs,
> addrcnt, SCTP_PARAM_ADD_IP);
> @@ -567,8 +563,6 @@ static int sctp_send_asconf_add_ip(struct sock *sk,
> /* Add the new addresses to the bind address list with
> * use_as_src set to 0.
> */
> - sctp_local_bh_disable();
> - sctp_write_lock(&asoc->base.addr_lock);
> addr_buf = addrs;
> for (i = 0; i < addrcnt; i++) {
> addr = (union sctp_addr *)addr_buf;
> @@ -578,8 +572,6 @@ static int sctp_send_asconf_add_ip(struct sock *sk,
> GFP_ATOMIC);
> addr_buf += af->sockaddr_len;
> }
> - sctp_write_unlock(&asoc->base.addr_lock);
> - sctp_local_bh_enable();
> }
>
> out:
> @@ -651,14 +643,8 @@ static int sctp_bindx_rem(struct sock *sk, struct sockaddr *addrs, int addrcnt)
> * socket routing and failover schemes. Refer to comments in
> * sctp_do_bind(). -daisy
> */
> - sctp_local_bh_disable();
> - sctp_write_lock(&ep->base.addr_lock);
> -
> retval = sctp_del_bind_addr(bp, sa_addr);
>
> - sctp_write_unlock(&ep->base.addr_lock);
> - sctp_local_bh_enable();
> -
> addr_buf += af->sockaddr_len;
> err_bindx_rem:
> if (retval < 0) {
> @@ -748,11 +734,9 @@ static int sctp_send_asconf_del_ip(struct sock *sk,
> * make sure that we do not delete all the addresses in the
> * association.
> */
> - sctp_read_lock(&asoc->base.addr_lock);
> bp = &asoc->base.bind_addr;
> laddr = sctp_find_unmatch_addr(bp, (union sctp_addr *)addrs,
> addrcnt, sp);
> - sctp_read_unlock(&asoc->base.addr_lock);
> if (!laddr)
> continue;
>
> @@ -766,23 +750,18 @@ static int sctp_send_asconf_del_ip(struct sock *sk,
> /* Reset use_as_src flag for the addresses in the bind address
> * list that are to be deleted.
> */
> - sctp_local_bh_disable();
> - sctp_write_lock(&asoc->base.addr_lock);
> addr_buf = addrs;
> for (i = 0; i < addrcnt; i++) {
> laddr = (union sctp_addr *)addr_buf;
> af = sctp_get_af_specific(laddr->v4.sin_family);
> - list_for_each(pos1, &bp->address_list) {
> - saddr = list_entry(pos1,
> - struct sctp_sockaddr_entry,
> - list);
> + rcu_read_lock();
> + list_for_each_entry_rcu(saddr, &bp->address_list, list) {
> if (sctp_cmp_addr_exact(&saddr->a, laddr))
> saddr->use_as_src = 0;
> }
> + rcu_read_unlock();
> addr_buf += af->sockaddr_len;
> }
> - sctp_write_unlock(&asoc->base.addr_lock);
> - sctp_local_bh_enable();
>
> /* Update the route and saddr entries for all the transports
> * as some of the addresses in the bind address list are
> @@ -4057,11 +4036,9 @@ static int sctp_getsockopt_local_addrs_num_old(struct sock *sk, int len,
> int __user *optlen)
> {
> sctp_assoc_t id;
> - struct list_head *pos;
> struct sctp_bind_addr *bp;
> struct sctp_association *asoc;
> struct sctp_sockaddr_entry *addr;
> - rwlock_t *addr_lock;
> int cnt = 0;
>
> if (len < sizeof(sctp_assoc_t))
> @@ -4078,17 +4055,13 @@ static int sctp_getsockopt_local_addrs_num_old(struct sock *sk, int len,
> */
> if (0 == id) {
> bp = &sctp_sk(sk)->ep->base.bind_addr;
> - addr_lock = &sctp_sk(sk)->ep->base.addr_lock;
> } else {
> asoc = sctp_id2assoc(sk, id);
> if (!asoc)
> return -EINVAL;
> bp = &asoc->base.bind_addr;
> - addr_lock = &asoc->base.addr_lock;
> }
>
> - sctp_read_lock(addr_lock);
> -
> /* If the endpoint is bound to 0.0.0.0 or ::0, count the valid
> * addresses from the global local address list.
> */
> @@ -4115,12 +4088,15 @@ static int sctp_getsockopt_local_addrs_num_old(struct sock *sk, int len,
> goto done;
> }
>
> - list_for_each(pos, &bp->address_list) {
> + rcu_read_lock();
> + list_for_each_entry_rcu(addr, &bp->address_list, list) {
> + if (!addr->valid)
> + continue;
> cnt ++;
> }
> + rcu_read_unlock();
>
> done:
> - sctp_read_unlock(addr_lock);
> return cnt;
> }
>
> @@ -4204,7 +4180,6 @@ static int sctp_getsockopt_local_addrs_old(struct sock *sk, int len,
> {
> struct sctp_bind_addr *bp;
> struct sctp_association *asoc;
> - struct list_head *pos;
> int cnt = 0;
> struct sctp_getaddrs_old getaddrs;
> struct sctp_sockaddr_entry *addr;
> @@ -4212,7 +4187,6 @@ static int sctp_getsockopt_local_addrs_old(struct sock *sk, int len,
> union sctp_addr temp;
> struct sctp_sock *sp = sctp_sk(sk);
> int addrlen;
> - rwlock_t *addr_lock;
> int err = 0;
> void *addrs;
> void *buf;
> @@ -4234,13 +4208,11 @@ static int sctp_getsockopt_local_addrs_old(struct sock *sk, int len,
> */
> if (0 == getaddrs.assoc_id) {
> bp = &sctp_sk(sk)->ep->base.bind_addr;
> - addr_lock = &sctp_sk(sk)->ep->base.addr_lock;
> } else {
> asoc = sctp_id2assoc(sk, getaddrs.assoc_id);
> if (!asoc)
> return -EINVAL;
> bp = &asoc->base.bind_addr;
> - addr_lock = &asoc->base.addr_lock;
> }
>
> to = getaddrs.addrs;
> @@ -4254,8 +4226,6 @@ static int sctp_getsockopt_local_addrs_old(struct sock *sk, int len,
> if (!addrs)
> return -ENOMEM;
>
> - sctp_read_lock(addr_lock);
> -
> /* If the endpoint is bound to 0.0.0.0 or ::0, get the valid
> * addresses from the global local address list.
> */
> @@ -4271,8 +4241,10 @@ static int sctp_getsockopt_local_addrs_old(struct sock *sk, int len,
> }
>
> buf = addrs;
> - list_for_each(pos, &bp->address_list) {
> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
> + rcu_read_lock();
> + list_for_each_entry_rcu(addr, &bp->address_list, list) {
> + if (!addr->valid)
> + continue;
> memcpy(&temp, &addr->a, sizeof(temp));
> sctp_get_pf_specific(sk->sk_family)->addr_v4map(sp, &temp);
> addrlen = sctp_get_af_specific(temp.sa.sa_family)->sockaddr_len;
> @@ -4282,10 +4254,9 @@ static int sctp_getsockopt_local_addrs_old(struct sock *sk, int len,
> cnt ++;
> if (cnt >= getaddrs.addr_num) break;
> }
> + rcu_read_unlock();
>
> copy_getaddrs:
> - sctp_read_unlock(addr_lock);
> -
> /* copy the entire address list into the user provided space */
> if (copy_to_user(to, addrs, bytes_copied)) {
> err = -EFAULT;
> @@ -4307,7 +4278,6 @@ static int sctp_getsockopt_local_addrs(struct sock *sk, int len,
> {
> struct sctp_bind_addr *bp;
> struct sctp_association *asoc;
> - struct list_head *pos;
> int cnt = 0;
> struct sctp_getaddrs getaddrs;
> struct sctp_sockaddr_entry *addr;
> @@ -4315,7 +4285,6 @@ static int sctp_getsockopt_local_addrs(struct sock *sk, int len,
> union sctp_addr temp;
> struct sctp_sock *sp = sctp_sk(sk);
> int addrlen;
> - rwlock_t *addr_lock;
> int err = 0;
> size_t space_left;
> int bytes_copied = 0;
> @@ -4336,13 +4305,11 @@ static int sctp_getsockopt_local_addrs(struct sock *sk, int len,
> */
> if (0 == getaddrs.assoc_id) {
> bp = &sctp_sk(sk)->ep->base.bind_addr;
> - addr_lock = &sctp_sk(sk)->ep->base.addr_lock;
> } else {
> asoc = sctp_id2assoc(sk, getaddrs.assoc_id);
> if (!asoc)
> return -EINVAL;
> bp = &asoc->base.bind_addr;
> - addr_lock = &asoc->base.addr_lock;
> }
>
> to = optval + offsetof(struct sctp_getaddrs,addrs);
> @@ -4352,8 +4319,6 @@ static int sctp_getsockopt_local_addrs(struct sock *sk, int len,
> if (!addrs)
> return -ENOMEM;
>
> - sctp_read_lock(addr_lock);
> -
> /* If the endpoint is bound to 0.0.0.0 or ::0, get the valid
> * addresses from the global local address list.
> */
> @@ -4372,8 +4337,10 @@ static int sctp_getsockopt_local_addrs(struct sock *sk, int len,
> }
>
> buf = addrs;
> - list_for_each(pos, &bp->address_list) {
> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
> + rcu_read_lock();
> + list_for_each_entry_rcu(addr, &bp->address_list, list) {
> + if (!addr->valid)
> + continue;
> memcpy(&temp, &addr->a, sizeof(temp));
> sctp_get_pf_specific(sk->sk_family)->addr_v4map(sp, &temp);
> addrlen = sctp_get_af_specific(temp.sa.sa_family)->sockaddr_len;
> @@ -4387,10 +4354,9 @@ static int sctp_getsockopt_local_addrs(struct sock *sk, int len,
> cnt ++;
> space_left -= addrlen;
> }
> + rcu_read_unlock();
>
> copy_getaddrs:
> - sctp_read_unlock(addr_lock);
> -
> if (copy_to_user(to, addrs, bytes_copied)) {
> err = -EFAULT;
> goto out;
> @@ -4405,7 +4371,7 @@ copy_getaddrs:
> goto out;
>
> error_lock:
> - sctp_read_unlock(addr_lock);
> + rcu_read_unlock();
>
> out:
> kfree(addrs);
> --
> 1.5.2.4
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists