On Wed, Mar 14, 2018 at 07:05:30PM +0800, Xin Long wrote:
> With refcnt support for sh_key, chunks auth sh_keys can be decided
> before enqueuing it. Changing the active key later will not affect
> the chunks already enqueued.
>
> Furthermore, this is necessary when adding the support for authinfo
> for sendmsg in next patch.
>
> Note that struct sctp_chunk can't be grown due to that performance
> drop issue on slow cpu, so it just reuses head_skb memory for shkey
> in sctp_chunk.
>
> Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> ---
> include/net/sctp/auth.h | 9 +++--
> include/net/sctp/sm.h | 3 +-
> include/net/sctp/structs.h | 9 +++--
> net/sctp/auth.c | 86 +++++++++++++++++++++++-----------------------
> net/sctp/chunk.c | 5 +++
> net/sctp/output.c | 18 ++++++++--
> net/sctp/sm_make_chunk.c | 15 ++++++--
> net/sctp/sm_statefuns.c | 11 +++---
> net/sctp/socket.c | 6 ++++
> 9 files changed, 104 insertions(+), 58 deletions(-)
>
> diff --git a/include/net/sctp/auth.h b/include/net/sctp/auth.h
> index e5c57d0..017c1aa 100644
> --- a/include/net/sctp/auth.h
> +++ b/include/net/sctp/auth.h
> @@ -62,8 +62,9 @@ struct sctp_auth_bytes {
> /* Definition for a shared key, weather endpoint or association */
> struct sctp_shared_key {
> struct list_head key_list;
> - __u16 key_id;
> struct sctp_auth_bytes *key;
> + refcount_t refcnt;
> + __u16 key_id;
> };
>
> #define key_for_each(__key, __list_head) \
> @@ -103,8 +104,10 @@ int sctp_auth_send_cid(enum sctp_cid chunk,
> int sctp_auth_recv_cid(enum sctp_cid chunk,
> const struct sctp_association *asoc);
> void sctp_auth_calculate_hmac(const struct sctp_association *asoc,
> - struct sk_buff *skb,
> - struct sctp_auth_chunk *auth, gfp_t gfp);
> + struct sk_buff *skb, struct sctp_auth_chunk *auth,
> + struct sctp_shared_key *ep_key, gfp_t gfp);
> +void sctp_auth_shkey_release(struct sctp_shared_key *sh_key);
> +void sctp_auth_shkey_hold(struct sctp_shared_key *sh_key);
>
> /* API Helpers */
> int sctp_auth_ep_add_chunkid(struct sctp_endpoint *ep, __u8 chunk_id);
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index 2883c43..2d0e782 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -263,7 +263,8 @@ int sctp_process_asconf_ack(struct sctp_association *asoc,
> struct sctp_chunk *sctp_make_fwdtsn(const struct sctp_association *asoc,
> __u32 new_cum_tsn, size_t nstreams,
> struct sctp_fwdtsn_skip *skiplist);
> -struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc);
> +struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc,
> + __u16 key_id);
> struct sctp_chunk *sctp_make_strreset_req(const struct sctp_association *asoc,
> __u16 stream_num, __be16 *stream_list,
> bool out, bool in);
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index ec6e46b..49ad67b 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -577,8 +577,12 @@ struct sctp_chunk {
> /* This points to the sk_buff containing the actual data. */
> struct sk_buff *skb;
>
> - /* In case of GSO packets, this will store the head one */
> - struct sk_buff *head_skb;
> + union {
> + /* In case of GSO packets, this will store the head one */
> + struct sk_buff *head_skb;
> + /* In case of auth enabled, this will point to the shkey */
> + struct sctp_shared_key *shkey;
> + };
Why do you need to add this at all? You add the shared key pointer to the
association in this patch, and sctp_chunk already has a pointer to the
association, so you should already be able to find the shared key via
chunk->asoc->shkey, no?
>
> /* These are the SCTP headers by reverse order in a packet.
> * Note that some of these may happen more than once. In that
> @@ -1995,6 +1999,7 @@ struct sctp_association {
> * The current generated assocaition shared key (secret)
> */
> struct sctp_auth_bytes *asoc_shared_key;
> + struct sctp_shared_key *shkey;
>
> /* SCTP AUTH: hmac id of the first peer requested algorithm
> * that we support.
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 00667c5..e5214fd 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -101,13 +101,14 @@ struct sctp_shared_key *sctp_auth_shkey_create(__u16 key_id, gfp_t gfp)
> return NULL;
>
> INIT_LIST_HEAD(&new->key_list);
> + refcount_set(&new->refcnt, 1);
> new->key_id = key_id;
>
> return new;
> }
>
> /* Free the shared key structure */
> -static void sctp_auth_shkey_free(struct sctp_shared_key *sh_key)
> +static void sctp_auth_shkey_destroy(struct sctp_shared_key *sh_key)
> {
> BUG_ON(!list_empty(&sh_key->key_list));
> sctp_auth_key_put(sh_key->key);
> @@ -115,6 +116,17 @@ static void sctp_auth_shkey_free(struct sctp_shared_key *sh_key)
> kfree(sh_key);
> }
>
> +void sctp_auth_shkey_release(struct sctp_shared_key *sh_key)
> +{
> + if (refcount_dec_and_test(&sh_key->refcnt))
> + sctp_auth_shkey_destroy(sh_key);
> +}
> +
> +void sctp_auth_shkey_hold(struct sctp_shared_key *sh_key)
> +{
> + refcount_inc(&sh_key->refcnt);
> +}
> +
> /* Destroy the entire key list. This is done during the
> * associon and endpoint free process.
> */
> @@ -128,7 +140,7 @@ void sctp_auth_destroy_keys(struct list_head *keys)
>
> key_for_each_safe(ep_key, tmp, keys) {
> list_del_init(&ep_key->key_list);
> - sctp_auth_shkey_free(ep_key);
> + sctp_auth_shkey_release(ep_key);
> }
> }
>
> @@ -409,13 +421,19 @@ int sctp_auth_asoc_init_active_key(struct sctp_association *asoc, gfp_t gfp)
>
> sctp_auth_key_put(asoc->asoc_shared_key);
> asoc->asoc_shared_key = secret;
> + asoc->shkey = ep_key;
>
> /* Update send queue in case any chunk already in there now
> * needs authenticating
> */
> list_for_each_entry(chunk, &asoc->outqueue.out_chunk_list, list) {
> - if (sctp_auth_send_cid(chunk->chunk_hdr->type, asoc))
> + if (sctp_auth_send_cid(chunk->chunk_hdr->type, asoc)) {
> chunk->auth = 1;
> + if (!chunk->shkey) {
> + chunk->shkey = asoc->shkey;
> + sctp_auth_shkey_hold(chunk->shkey);
> + }
> + }
Do we really need to take a reference for every chunk we send? Wouldn't it be
sufficient to just hold the shared key until the association is released?
> }
>
> return 0;
> @@ -703,16 +721,15 @@ int sctp_auth_recv_cid(enum sctp_cid chunk, const struct sctp_association *asoc)
> * after the AUTH chunk in the SCTP packet.
> */
> void sctp_auth_calculate_hmac(const struct sctp_association *asoc,
> - struct sk_buff *skb,
> - struct sctp_auth_chunk *auth,
> - gfp_t gfp)
> + struct sk_buff *skb, struct sctp_auth_chunk *auth,
> + struct sctp_shared_key *ep_key, gfp_t gfp)
> {
> - struct crypto_shash *tfm;
> struct sctp_auth_bytes *asoc_key;
> + struct crypto_shash *tfm;
> __u16 key_id, hmac_id;
> - __u8 *digest;
> unsigned char *end;
> int free_key = 0;
> + __u8 *digest;
>
> /* Extract the info we need:
> * - hmac id
> @@ -724,12 +741,7 @@ void sctp_auth_calculate_hmac(const struct sctp_association *asoc,
> if (key_id == asoc->active_key_id)
> asoc_key = asoc->asoc_shared_key;
> else {
> - struct sctp_shared_key *ep_key;
> -
> - ep_key = sctp_auth_get_shkey(asoc, key_id);
> - if (!ep_key)
> - return;
> -
> + /* ep_key can't be NULL here */
> asoc_key = sctp_auth_asoc_create_secret(asoc, ep_key, gfp);
> if (!asoc_key)
> return;
> @@ -829,7 +841,7 @@ int sctp_auth_set_key(struct sctp_endpoint *ep,
> struct sctp_association *asoc,
> struct sctp_authkey *auth_key)
> {
> - struct sctp_shared_key *cur_key = NULL;
> + struct sctp_shared_key *cur_key, *shkey;
> struct sctp_auth_bytes *key;
> struct list_head *sh_keys;
> int replace = 0;
> @@ -842,46 +854,34 @@ int sctp_auth_set_key(struct sctp_endpoint *ep,
> else
> sh_keys = &ep->endpoint_shared_keys;
>
> - key_for_each(cur_key, sh_keys) {
> - if (cur_key->key_id == auth_key->sca_keynumber) {
> + key_for_each(shkey, sh_keys) {
> + if (shkey->key_id == auth_key->sca_keynumber) {
> replace = 1;
> break;
> }
> }
>
> - /* If we are not replacing a key id, we need to allocate
> - * a shared key.
> - */
> - if (!replace) {
> - cur_key = sctp_auth_shkey_create(auth_key->sca_keynumber,
> - GFP_KERNEL);
> - if (!cur_key)
> - return -ENOMEM;
> - }
> + cur_key = sctp_auth_shkey_create(auth_key->sca_keynumber, GFP_KERNEL);
> + if (!cur_key)
> + return -ENOMEM;
>
> /* Create a new key data based on the info passed in */
> key = sctp_auth_create_key(auth_key->sca_keylength, GFP_KERNEL);
> - if (!key)
> - goto nomem;
> + if (!key) {
> + kfree(cur_key);
> + return -ENOMEM;
> + }
>
> memcpy(key->data, &auth_key->sca_key[0], auth_key->sca_keylength);
> + cur_key->key = key;
>
> - /* If we are replacing, remove the old keys data from the
> - * key id. If we are adding new key id, add it to the
> - * list.
> - */
> - if (replace)
> - sctp_auth_key_put(cur_key->key);
> - else
> - list_add(&cur_key->key_list, sh_keys);
> + if (replace) {
> + list_del_init(&shkey->key_list);
> + sctp_auth_shkey_release(shkey);
> + }
> + list_add(&cur_key->key_list, sh_keys);
>
> - cur_key->key = key;
> return 0;
> -nomem:
> - if (!replace)
> - sctp_auth_shkey_free(cur_key);
> -
> - return -ENOMEM;
> }
>
> int sctp_auth_set_active_key(struct sctp_endpoint *ep,
> @@ -952,7 +952,7 @@ int sctp_auth_del_key_id(struct sctp_endpoint *ep,
>
> /* Delete the shared key */
> list_del_init(&key->key_list);
> - sctp_auth_shkey_free(key);
> + sctp_auth_shkey_release(key);
>
> return 0;
> }
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index 991a530..9f28a9a 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -168,6 +168,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> {
> size_t len, first_len, max_data, remaining;
> size_t msg_len = iov_iter_count(from);
> + struct sctp_shared_key *shkey = NULL;
> struct list_head *pos, *temp;
> struct sctp_chunk *chunk;
> struct sctp_datamsg *msg;
> @@ -204,6 +205,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> if (hmac_desc)
> max_data -= SCTP_PAD4(sizeof(struct sctp_auth_chunk) +
> hmac_desc->hmac_len);
> +
> + shkey = asoc->shkey;
> }
>
> /* Check what's our max considering the above */
> @@ -275,6 +278,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> if (err < 0)
> goto errout_chunk_free;
>
> + chunk->shkey = shkey;
> +
> /* Put the chunk->skb back into the form expected by send. */
> __skb_pull(chunk->skb, (__u8 *)chunk->chunk_hdr -
> chunk->skb->data);
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 01a26ee0..d6e1c90 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -241,10 +241,13 @@ static enum sctp_xmit sctp_packet_bundle_auth(struct sctp_packet *pkt,
> if (!chunk->auth)
> return retval;
>
> - auth = sctp_make_auth(asoc);
> + auth = sctp_make_auth(asoc, chunk->shkey->key_id);
> if (!auth)
> return retval;
>
> + auth->shkey = chunk->shkey;
> + sctp_auth_shkey_hold(auth->shkey);
> +
> retval = __sctp_packet_append_chunk(pkt, auth);
>
> if (retval != SCTP_XMIT_OK)
> @@ -490,7 +493,8 @@ static int sctp_packet_pack(struct sctp_packet *packet,
> }
>
> if (auth) {
> - sctp_auth_calculate_hmac(tp->asoc, nskb, auth, gfp);
> + sctp_auth_calculate_hmac(tp->asoc, nskb, auth,
> + packet->auth->shkey, gfp);
> /* free auth if no more chunks, or add it back */
> if (list_empty(&packet->chunk_list))
> sctp_chunk_free(packet->auth);
> @@ -770,6 +774,16 @@ static enum sctp_xmit sctp_packet_will_fit(struct sctp_packet *packet,
> enum sctp_xmit retval = SCTP_XMIT_OK;
> size_t psize, pmtu, maxsize;
>
> + /* Don't bundle in this packet if this chunk's auth key doesn't
> + * match other chunks already enqueued on this packet. Also,
> + * don't bundle the chunk with auth key if other chunks in this
> + * packet don't have auth key.
> + */
> + if ((packet->auth && chunk->shkey != packet->auth->shkey) ||
> + (!packet->auth && chunk->shkey &&
> + chunk->chunk_hdr->type != SCTP_CID_AUTH))
> + return SCTP_XMIT_PMTU_FULL;
> +
> psize = packet->size;
> if (packet->transport->asoc)
> pmtu = packet->transport->asoc->pathmtu;
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index d01475f..10f071c 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -87,7 +87,10 @@ static void *sctp_addto_chunk_fixed(struct sctp_chunk *, int len,
> /* Control chunk destructor */
> static void sctp_control_release_owner(struct sk_buff *skb)
> {
> - /*TODO: do memory release */
> + struct sctp_chunk *chunk = skb_shinfo(skb)->destructor_arg;
> +
> + if (chunk->shkey)
> + sctp_auth_shkey_release(chunk->shkey);
> }
>
> static void sctp_control_set_owner_w(struct sctp_chunk *chunk)
> @@ -102,7 +105,12 @@ static void sctp_control_set_owner_w(struct sctp_chunk *chunk)
> *
> * For now don't do anything for now.
> */
> + if (chunk->auth) {
> + chunk->shkey = asoc->shkey;
> + sctp_auth_shkey_hold(chunk->shkey);
> + }
> skb->sk = asoc ? asoc->base.sk : NULL;
> + skb_shinfo(skb)->destructor_arg = chunk;
> skb->destructor = sctp_control_release_owner;
> }
>
> @@ -1271,7 +1279,8 @@ struct sctp_chunk *sctp_make_op_error(const struct sctp_association *asoc,
> return retval;
> }
>
> -struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc)
> +struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc,
> + __u16 key_id)
> {
> struct sctp_authhdr auth_hdr;
> struct sctp_hmac *hmac_desc;
> @@ -1289,7 +1298,7 @@ struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc)
> return NULL;
>
> auth_hdr.hmac_id = htons(hmac_desc->hmac_id);
> - auth_hdr.shkey_id = htons(asoc->active_key_id);
> + auth_hdr.shkey_id = htons(key_id);
>
> retval->subh.auth_hdr = sctp_addto_chunk(retval, sizeof(auth_hdr),
> &auth_hdr);
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index eb7905f..792e0e2 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -4114,6 +4114,7 @@ static enum sctp_ierror sctp_sf_authenticate(
> const union sctp_subtype type,
> struct sctp_chunk *chunk)
> {
> + struct sctp_shared_key *sh_key = NULL;
> struct sctp_authhdr *auth_hdr;
> __u8 *save_digest, *digest;
> struct sctp_hmac *hmac;
> @@ -4135,9 +4136,11 @@ static enum sctp_ierror sctp_sf_authenticate(
> * configured
> */
> key_id = ntohs(auth_hdr->shkey_id);
> - if (key_id != asoc->active_key_id && !sctp_auth_get_shkey(asoc, key_id))
> - return SCTP_IERROR_AUTH_BAD_KEYID;
> -
> + if (key_id != asoc->active_key_id) {
> + sh_key = sctp_auth_get_shkey(asoc, key_id);
> + if (!sh_key)
> + return SCTP_IERROR_AUTH_BAD_KEYID;
> + }
>
> /* Make sure that the length of the signature matches what
> * we expect.
> @@ -4166,7 +4169,7 @@ static enum sctp_ierror sctp_sf_authenticate(
>
> sctp_auth_calculate_hmac(asoc, chunk->skb,
> (struct sctp_auth_chunk *)chunk->chunk_hdr,
> - GFP_ATOMIC);
> + sh_key, GFP_ATOMIC);
>
> /* Discard the packet if the digests do not match */
> if (memcmp(save_digest, digest, sig_len)) {
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index af5cf29..003a4ad 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -156,6 +156,9 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk)
> /* The sndbuf space is tracked per association. */
> sctp_association_hold(asoc);
>
> + if (chunk->shkey)
> + sctp_auth_shkey_hold(chunk->shkey);
> +
> skb_set_owner_w(chunk->skb, sk);
>
> chunk->skb->destructor = sctp_wfree;
> @@ -8109,6 +8112,9 @@ static void sctp_wfree(struct sk_buff *skb)
> sk->sk_wmem_queued -= skb->truesize;
> sk_mem_uncharge(sk, skb->truesize);
>
> + if (chunk->shkey)
> + sctp_auth_shkey_release(chunk->shkey);
> +
> sock_wfree(skb);
> sctp_wake_up_waiters(sk, asoc);
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
