Re: [PATCH net-next 1/5] sctp: add refcnt support for sh_key

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Thu, Mar 15, 2018 at 12:12:32AM +0800, Xin Long wrote:
> On Wed, Mar 14, 2018 at 9:59 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > 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@gmail.com>
> >> ---
> >>  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?
> We need this, because one asoc can have a list of shared keys.
> When sending a msg, we can even choose one of these keys with
> cmsg info (cmsgs->authinfo) for this msg, which is that
> 5.3.8.  SCTP AUTH Information Structure (SCTP_AUTHINFO)
> is supposed to do. (Patch 2/5)
> 
> After this patchset, asoc->shkey (or we can say active shkey) is
> more like default shkey. It will be used only when SCTP_AUTHINFO
> is not set in cmsg info.

And even then, now once the chunk is enqueued, asoc->shkey is allowed
to change without affecting it.

This is probably one of the main improvements on this patchset.

  M.