On Thu, 2018-03-22 at 01:27 +0300, Kirill Tkhai wrote: > Hi, Saeed, > > thanks for fixing some of my remarks, but I've dived into the code > more deeply, and found with a sadness, the patch lacks the > readability. > > It too big and not fit kernel coding style. Please, see some comments > below. >
Hi Kirill, I don't see any reason for this, there are two major files, they are not that big and the code is really straight forward. net/tls/tls_device.c (tls netdevice offload) net/tls/tls_device_fallback.c (tls software offload) they are nicely decoupled and and you can review each file separately. Is it really bothering you that much ? Anyway I will submit v3 with the fixes (from Boris) of the below comments. > Can we do something with patch length? Is there a way to split it in > several small patches? It's difficult to review the logic of changes. > > On 22.03.2018 00:01, Saeed Mahameed wrote: > > From: Ilya Lesokhin <il...@mellanox.com> > > > > This patch adds a generic infrastructure to offload TLS crypto to a > > network devices. It enables the kernel TLS socket to skip > > encryption > > and authentication operations on the transmit side of the data > > path. > > Leaving those computationally expensive operations to the NIC. > > > > The NIC offload infrastructure builds TLS records and pushes them > > to > > the TCP layer just like the SW KTLS implementation and using the > > same API. > > TCP segmentation is mostly unaffected. Currently the only exception > > is > > that we prevent mixed SKBs where only part of the payload requires > > offload. In the future we are likely to add a similar restriction > > following a change cipher spec record. > > > > The notable differences between SW KTLS and NIC offloaded TLS > > implementations are as follows: > > 1. The offloaded implementation builds "plaintext TLS record", > > those > > records contain plaintext instead of ciphertext and place holder > > bytes > > instead of authentication tags. > > 2. The offloaded implementation maintains a mapping from TCP > > sequence > > number to TLS records. Thus given a TCP SKB sent from a NIC > > offloaded > > TLS socket, we can use the tls NIC offload infrastructure to obtain > > enough context to encrypt the payload of the SKB. > > A TLS record is released when the last byte of the record is > > ack'ed, > > this is done through the new icsk_clean_acked callback. > > > > The infrastructure should be extendable to support various NIC > > offload > > implementations. However it is currently written with the > > implementation below in mind: > > The NIC assumes that packets from each offloaded stream are sent as > > plaintext and in-order. It keeps track of the TLS records in the > > TCP > > stream. When a packet marked for offload is transmitted, the NIC > > encrypts the payload in-place and puts authentication tags in the > > relevant place holders. > > > > The responsibility for handling out-of-order packets (i.e. TCP > > retransmission, qdisc drops) falls on the netdev driver. > > > > The netdev driver keeps track of the expected TCP SN from the NIC's > > perspective. If the next packet to transmit matches the expected > > TCP > > SN, the driver advances the expected TCP SN, and transmits the > > packet > > with TLS offload indication. > > > > If the next packet to transmit does not match the expected TCP SN. > > The > > driver calls the TLS layer to obtain the TLS record that includes > > the > > TCP of the packet for transmission. Using this TLS record, the > > driver > > posts a work entry on the transmit queue to reconstruct the NIC TLS > > state required for the offload of the out-of-order packet. It > > updates > > the expected TCP SN accordingly and transmit the now in-order > > packet. > > The same queue is used for packet transmission and TLS context > > reconstruction to avoid the need for flushing the transmit queue > > before > > issuing the context reconstruction request. > > > > Signed-off-by: Ilya Lesokhin <il...@mellanox.com> > > Signed-off-by: Boris Pismenny <bor...@mellanox.com> > > Signed-off-by: Aviad Yehezkel <avia...@mellanox.com> > > Signed-off-by: Saeed Mahameed <sae...@mellanox.com> > > --- > > include/net/tls.h | 74 +++- > > net/tls/Kconfig | 10 + > > net/tls/Makefile | 2 + > > net/tls/tls_device.c | 793 > > ++++++++++++++++++++++++++++++++++++++++++ > > net/tls/tls_device_fallback.c | 415 ++++++++++++++++++++++ > > net/tls/tls_main.c | 33 +- > > 6 files changed, 1320 insertions(+), 7 deletions(-) > > create mode 100644 net/tls/tls_device.c > > create mode 100644 net/tls/tls_device_fallback.c > > > > diff --git a/include/net/tls.h b/include/net/tls.h > > index 4913430ab807..0bfb1b0a156a 100644 > > --- a/include/net/tls.h > > +++ b/include/net/tls.h > > @@ -77,6 +77,37 @@ struct tls_sw_context { > > struct scatterlist sg_aead_out[2]; > > }; > > > > +struct tls_record_info { > > + struct list_head list; > > + u32 end_seq; > > + int len; > > + int num_frags; > > + skb_frag_t frags[MAX_SKB_FRAGS]; > > +}; > > + > > +struct tls_offload_context { > > + struct crypto_aead *aead_send; > > + spinlock_t lock; /* protects records list */ > > + struct list_head records_list; > > + struct tls_record_info *open_record; > > + struct tls_record_info *retransmit_hint; > > + u64 hint_record_sn; > > + u64 unacked_record_sn; > > + > > + struct scatterlist sg_tx_data[MAX_SKB_FRAGS]; > > + void (*sk_destruct)(struct sock *sk); > > + u8 driver_state[]; > > + /* The TLS layer reserves room for driver specific state > > + * Currently the belief is that there is not enough > > + * driver specific state to justify another layer of > > indirection > > + */ > > +#define TLS_DRIVER_STATE_SIZE (max_t(size_t, 8, sizeof(void *))) > > +}; > > + > > +#define > > TLS_OFFLOAD_CONTEXT_SIZE > > \ > > + (ALIGN(sizeof(struct tls_offload_context), sizeof(void *)) > > + \ > > + TLS_DRIVER_STATE_SIZE) > > + > > enum { > > TLS_PENDING_CLOSED_RECORD > > }; > > @@ -87,6 +118,10 @@ struct tls_context { > > struct tls12_crypto_info_aes_gcm_128 > > crypto_send_aes_gcm_128; > > }; > > > > + struct list_head list; > > + struct net_device *netdev; > > + refcount_t refcount; > > + > > void *priv_ctx; > > > > u8 tx_conf:2; > > @@ -131,9 +166,29 @@ int tls_sw_sendpage(struct sock *sk, struct > > page *page, > > void tls_sw_close(struct sock *sk, long timeout); > > void tls_sw_free_tx_resources(struct sock *sk); > > > > -void tls_sk_destruct(struct sock *sk, struct tls_context *ctx); > > -void tls_icsk_clean_acked(struct sock *sk); > > +void tls_clear_device_offload(struct sock *sk, struct tls_context > > *ctx); > > +int tls_set_device_offload(struct sock *sk, struct tls_context > > *ctx); > > +int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t > > size); > > +int tls_device_sendpage(struct sock *sk, struct page *page, > > + int offset, size_t size, int flags); > > +void tls_device_sk_destruct(struct sock *sk); > > +void tls_device_init(void); > > +void tls_device_cleanup(void); > > + > > +struct tls_record_info *tls_get_record(struct tls_offload_context > > *context, > > + u32 seq, u64 *p_record_sn); > > + > > +static inline bool tls_record_is_start_marker(struct > > tls_record_info *rec) > > +{ > > + return rec->len == 0; > > +} > > + > > +static inline u32 tls_record_start_seq(struct tls_record_info > > *rec) > > +{ > > + return rec->end_seq - rec->len; > > +} > > > > +void tls_sk_destruct(struct sock *sk, struct tls_context *ctx); > > int tls_push_sg(struct sock *sk, struct tls_context *ctx, > > struct scatterlist *sg, u16 first_offset, > > int flags); > > @@ -170,6 +225,13 @@ static inline bool > > tls_is_pending_open_record(struct tls_context *tls_ctx) > > return tls_ctx->pending_open_record_frags; > > } > > > > +static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk) > > +{ > > + return sk_fullsock(sk) && > > + /* matches smp_store_release in > > tls_set_device_offload */ > > + smp_load_acquire(&sk->sk_destruct) == > > &tls_device_sk_destruct; > > +} > > + > > static inline void tls_err_abort(struct sock *sk) > > { > > sk->sk_err = EBADMSG; > > @@ -257,4 +319,12 @@ static inline struct tls_offload_context > > *tls_offload_ctx( > > int tls_proccess_cmsg(struct sock *sk, struct msghdr *msg, > > unsigned char *record_type); > > > > +struct sk_buff *tls_validate_xmit_skb(struct sock *sk, > > + struct net_device *dev, > > + struct sk_buff *skb); > > + > > +int tls_sw_fallback_init(struct sock *sk, > > + struct tls_offload_context *offload_ctx, > > + struct tls_crypto_info *crypto_info); > > + > > #endif /* _TLS_OFFLOAD_H */ > > diff --git a/net/tls/Kconfig b/net/tls/Kconfig > > index eb583038c67e..9d3ef820bb16 100644 > > --- a/net/tls/Kconfig > > +++ b/net/tls/Kconfig > > @@ -13,3 +13,13 @@ config TLS > > encryption handling of the TLS protocol to be done in- > > kernel. > > > > If unsure, say N. > > + > > +config TLS_DEVICE > > + bool "Transport Layer Security HW offload" > > + depends on TLS > > + select SOCK_VALIDATE_XMIT > > + default n > > + ---help--- > > + Enable kernel support for HW offload of the TLS protocol. > > + > > + If unsure, say N. > > diff --git a/net/tls/Makefile b/net/tls/Makefile > > index a930fd1c4f7b..4d6b728a67d0 100644 > > --- a/net/tls/Makefile > > +++ b/net/tls/Makefile > > @@ -5,3 +5,5 @@ > > obj-$(CONFIG_TLS) += tls.o > > > > tls-y := tls_main.o tls_sw.o > > + > > +tls-$(CONFIG_TLS_DEVICE) += tls_device.o tls_device_fallback.o > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c > > new file mode 100644 > > index 000000000000..e623280ea019 > > --- /dev/null > > +++ b/net/tls/tls_device.c > > @@ -0,0 +1,793 @@ > > +/* Copyright (c) 2018, Mellanox Technologies All rights reserved. > > + * > > + * This software is available to you under a choice of one of two > > + * licenses. You may choose to be licensed under the terms of the > > GNU > > + * General Public License (GPL) Version 2, available from the file > > + * COPYING in the main directory of this source tree, or the > > + * OpenIB.org BSD license below: > > + * > > + * Redistribution and use in source and binary forms, with or > > + * without modification, are permitted provided that the > > following > > + * conditions are met: > > + * > > + * - Redistributions of source code must retain the above > > + * copyright notice, this list of conditions and the > > following > > + * disclaimer. > > + * > > + * - Redistributions in binary form must reproduce the above > > + * copyright notice, this list of conditions and the > > following > > + * disclaimer in the documentation and/or other materials > > + * provided with the distribution. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES > > OF > > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT > > HOLDERS > > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN > > AN > > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR > > IN > > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > > THE > > + * SOFTWARE. > > + */ > > + > > +#include <linux/module.h> > > +#include <net/tcp.h> > > +#include <net/inet_common.h> > > +#include <linux/highmem.h> > > +#include <linux/netdevice.h> > > + > > +#include <net/tls.h> > > +#include <crypto/aead.h> > > + > > +/* device_offload_lock is used to synchronize tls_dev_add > > + * against NETDEV_DOWN notifications. > > + */ > > +static DECLARE_RWSEM(device_offload_lock); > > + > > +static void tls_device_gc_task(struct work_struct *work); > > + > > +static DECLARE_WORK(tls_device_gc_work, tls_device_gc_task); > > +static LIST_HEAD(tls_device_gc_list); > > +static LIST_HEAD(tls_device_list); > > +static DEFINE_SPINLOCK(tls_device_lock); > > + > > +static void tls_device_free_ctx(struct tls_context *ctx) > > +{ > > + struct tls_offload_context *offlad_ctx = > > tls_offload_ctx(ctx); > > + > > + kfree(offlad_ctx); > > + kfree(ctx); > > +} > > + > > +static void tls_device_gc_task(struct work_struct *work) > > +{ > > + struct tls_context *ctx, *tmp; > > + struct list_head gc_list; > > + unsigned long flags; > > + > > + INIT_LIST_HEAD(&gc_list); > > You should just declare the list as > > LIST_HEAD(gc_list); > > I've already pointed this in commentary to v1. > > > + > > + spin_lock_irqsave(&tls_device_lock, flags); > > + list_splice_init(&tls_device_gc_list, &gc_list); > > + spin_unlock_irqrestore(&tls_device_lock, flags); > > + > > + list_for_each_entry_safe(ctx, tmp, &gc_list, list) { > > + struct net_device *netdev = ctx->netdev; > > + > > + if (netdev) { > > + netdev->tlsdev_ops->tls_dev_del(netdev, > > ctx, > > + TLS_OFFLOA > > D_CTX_DIR_TX); > > + dev_put(netdev); > > + } > > + > > + list_del(&ctx->list); > > + tls_device_free_ctx(ctx); > > + } > > +} > > + > > +static void tls_device_queue_ctx_destruction(struct tls_context > > *ctx) > > +{ > > + unsigned long flags; > > + > > + spin_lock_irqsave(&tls_device_lock, flags); > > + list_move_tail(&ctx->list, &tls_device_gc_list); > > + > > + /* schedule_work inside the spinlock > > + * to make sure tls_device_down waits for that work. > > + */ > > + schedule_work(&tls_device_gc_work); > > + > > + spin_unlock_irqrestore(&tls_device_lock, flags); > > +} > > + > > +/* We assume that the socket is already connected */ > > +static struct net_device *get_netdev_for_sock(struct sock *sk) > > +{ > > + struct inet_sock *inet = inet_sk(sk); > > + struct net_device *netdev = NULL; > > + > > + netdev = dev_get_by_index(sock_net(sk), inet- > > >cork.fl.flowi_oif); > > + > > + return netdev; > > Why can't we just return > > return dev_get_by_index(sock_net(sk), inet->cork.fl.flowi_oif); > ? > > What for we need this netdev declaration? The direct return makes the > line > even shorter. In the next patch this function will change to support IPv6. > > > +} > > + > > +static int attach_sock_to_netdev(struct sock *sk, struct > > net_device *netdev, > > + struct tls_context *ctx) > > +{ > > + int rc; > > + > > + rc = netdev->tlsdev_ops->tls_dev_add(netdev, sk, > > TLS_OFFLOAD_CTX_DIR_TX, > > + &ctx->crypto_send, > > + tcp_sk(sk)- > > >write_seq); > > + if (rc) { > > + pr_err_ratelimited("The netdev has refused to > > offload this socket\n"); > > + goto out; > > + } > > + > > + rc = 0; > > +out: > > + return rc; > > > Too many not functional/useless lines. What for is this out label??? > This has to be: > > rc = netdev->tlsdev_ops->tls_dev_add(netdev, sk, > TLS_OFFLOAD_CTX_DIR_TX, > &ctx->crypto_send, > tcp_sk(sk)->write_seq); > if (rc) > pr_err_ratelimited("The netdev has refused to offload > this socket\n"); > > return rc; > > > +} > > + > > +static void destroy_record(struct tls_record_info *record) > > +{ > > + skb_frag_t *frag; > > + int nr_frags = record->num_frags; > > + > > + while (nr_frags > 0) { > > + frag = &record->frags[nr_frags - 1]; > > + __skb_frag_unref(frag); > > + --nr_frags; > > Why just not to write the below instead? > > while (nr_frags-- > 0) { > frag = &record->frags[nr_frags]; > __skb_frag_unref(frag); > } > > > + } > > + kfree(record); > > +} > > + > > +static void delete_all_records(struct tls_offload_context > > *offload_ctx) > > +{ > > + struct tls_record_info *info, *temp; > > + > > + list_for_each_entry_safe(info, temp, &offload_ctx- > > >records_list, list) { > > + list_del(&info->list); > > + destroy_record(info); > > + } > > + > > + offload_ctx->retransmit_hint = NULL; > > +} > > + > > +static void tls_icsk_clean_acked(struct sock *sk, u32 acked_seq) > > +{ > > + struct tls_context *tls_ctx = tls_get_ctx(sk); > > + struct tls_offload_context *ctx; > > + struct tls_record_info *info, *temp; > > + unsigned long flags; > > + u64 deleted_records = 0; > > + > > + if (!tls_ctx) > > + return; > > + > > + ctx = tls_offload_ctx(tls_ctx); > > + > > + spin_lock_irqsave(&ctx->lock, flags); > > + info = ctx->retransmit_hint; > > + if (info && !before(acked_seq, info->end_seq)) { > > + ctx->retransmit_hint = NULL; > > + list_del(&info->list); > > + destroy_record(info); > > + deleted_records++; > > + } > > + > > + list_for_each_entry_safe(info, temp, &ctx->records_list, > > list) { > > + if (before(acked_seq, info->end_seq)) > > + break; > > + list_del(&info->list); > > + > > + destroy_record(info); > > + deleted_records++; > > + } > > + > > + ctx->unacked_record_sn += deleted_records; > > + spin_unlock_irqrestore(&ctx->lock, flags); > > +} > > + > > +/* At this point, there should be no references on this > > + * socket and no in-flight SKBs associated with this > > + * socket, so it is safe to free all the resources. > > + */ > > +void tls_device_sk_destruct(struct sock *sk) > > +{ > > + struct tls_context *tls_ctx = tls_get_ctx(sk); > > + struct tls_offload_context *ctx = > > tls_offload_ctx(tls_ctx); > > + > > + if (ctx->open_record) > > + destroy_record(ctx->open_record); > > + > > + delete_all_records(ctx); > > + crypto_free_aead(ctx->aead_send); > > + ctx->sk_destruct(sk); > > + static_branch_dec(&clean_acked_data_enabled); > > + > > + if (refcount_dec_and_test(&tls_ctx->refcount)) > > + tls_device_queue_ctx_destruction(tls_ctx); > > +} > > +EXPORT_SYMBOL(tls_device_sk_destruct); > > + > > +static inline void tls_append_frag(struct tls_record_info *record, > > + struct page_frag *pfrag, > > + int size) > > +{ > > + skb_frag_t *frag; > > + > > + frag = &record->frags[record->num_frags - 1]; > > + if (frag->page.p == pfrag->page && > > + frag->page_offset + frag->size == pfrag->offset) { > > + frag->size += size; > > + } else { > > + ++frag; > > + frag->page.p = pfrag->page; > > + frag->page_offset = pfrag->offset; > > + frag->size = size; > > + ++record->num_frags; > > + get_page(pfrag->page); > > + } > > + > > + pfrag->offset += size; > > + record->len += size; > > +} > > + > > +static inline int tls_push_record(struct sock *sk, > > + struct tls_context *ctx, > > + struct tls_offload_context > > *offload_ctx, > > + struct tls_record_info *record, > > + struct page_frag *pfrag, > > + int flags, > > + unsigned char record_type) > > +{ > > + skb_frag_t *frag; > > + struct tcp_sock *tp = tcp_sk(sk); > > + struct page_frag fallback_frag; > > + struct page_frag *tag_pfrag = pfrag; > > + int i; > > + > > + /* fill prepand */ > > + frag = &record->frags[0]; > > + tls_fill_prepend(ctx, > > + skb_frag_address(frag), > > + record->len - ctx->prepend_size, > > + record_type); > > + > > + if (unlikely(!skb_page_frag_refill(ctx->tag_size, pfrag, > > GFP_KERNEL))) { > > + /* HW doesn't care about the data in the tag > > + * so in case pfrag has no room > > + * for a tag and we can't allocate a new pfrag > > + * just use the page in the first frag > > + * rather then write a complicated fall back code. > > + */ > > + tag_pfrag = &fallback_frag; > > + tag_pfrag->page = skb_frag_page(frag); > > + tag_pfrag->offset = 0; > > + } > > + > > + tls_append_frag(record, tag_pfrag, ctx->tag_size); > > + record->end_seq = tp->write_seq + record->len; > > + spin_lock_irq(&offload_ctx->lock); > > + list_add_tail(&record->list, &offload_ctx->records_list); > > + spin_unlock_irq(&offload_ctx->lock); > > + offload_ctx->open_record = NULL; > > + set_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags); > > + tls_advance_record_sn(sk, ctx); > > + > > + for (i = 0; i < record->num_frags; i++) { > > + frag = &record->frags[i]; > > + sg_unmark_end(&offload_ctx->sg_tx_data[i]); > > + sg_set_page(&offload_ctx->sg_tx_data[i], > > skb_frag_page(frag), > > + frag->size, frag->page_offset); > > + sk_mem_charge(sk, frag->size); > > + get_page(skb_frag_page(frag)); > > + } > > + sg_mark_end(&offload_ctx->sg_tx_data[record->num_frags - > > 1]); > > + > > + /* all ready, send */ > > + return tls_push_sg(sk, ctx, offload_ctx->sg_tx_data, 0, > > flags); > > +} > > + > > +static inline int tls_create_new_record(struct tls_offload_context > > *offload_ctx, > > + struct page_frag *pfrag, > > + size_t prepend_size) > > +{ > > + skb_frag_t *frag; > > + struct tls_record_info *record; > > + > > + record = kmalloc(sizeof(*record), GFP_KERNEL); > > + if (!record) > > + return -ENOMEM; > > + > > + frag = &record->frags[0]; > > + __skb_frag_set_page(frag, pfrag->page); > > + frag->page_offset = pfrag->offset; > > + skb_frag_size_set(frag, prepend_size); > > + > > + get_page(pfrag->page); > > + pfrag->offset += prepend_size; > > + > > + record->num_frags = 1; > > + record->len = prepend_size; > > + offload_ctx->open_record = record; > > + return 0; > > +} > > + > > +static inline int tls_do_allocation(struct sock *sk, > > + struct tls_offload_context > > *offload_ctx, > > + struct page_frag *pfrag, > > + size_t prepend_size) > > +{ > > + int ret; > > + > > + if (!offload_ctx->open_record) { > > + if (unlikely(!skb_page_frag_refill(prepend_size, > > pfrag, > > + sk- > > >sk_allocation))) { > > + sk->sk_prot->enter_memory_pressure(sk); > > + sk_stream_moderate_sndbuf(sk); > > + return -ENOMEM; > > + } > > + > > + ret = tls_create_new_record(offload_ctx, pfrag, > > prepend_size); > > + if (ret) > > + return ret; > > + > > + if (pfrag->size > pfrag->offset) > > + return 0; > > + } > > + > > + if (!sk_page_frag_refill(sk, pfrag)) > > + return -ENOMEM; > > + > > + return 0; > > +} > > + > > +static int tls_push_data(struct sock *sk, > > + struct iov_iter *msg_iter, > > + size_t size, int flags, > > + unsigned char record_type) > > +{ > > + struct tls_context *tls_ctx = tls_get_ctx(sk); > > + struct tls_offload_context *ctx = > > tls_offload_ctx(tls_ctx); > > + struct tls_record_info *record = ctx->open_record; > > + struct page_frag *pfrag; > > + int copy, rc = 0; > > + size_t orig_size = size; > > + u32 max_open_record_len; > > + long timeo; > > + int more = flags & (MSG_SENDPAGE_NOTLAST | MSG_MORE); > > + int tls_push_record_flags = flags | MSG_SENDPAGE_NOTLAST; > > + bool done = false; > > As David says, variables have to be declared in reverse Сhristmas > tree order. > > > + > > + if (flags & > > + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | > > MSG_SENDPAGE_NOTLAST)) > > + return -ENOTSUPP; > > + > > + if (sk->sk_err) > > + return -sk->sk_err; > > + > > + timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT); > > + rc = tls_complete_pending_work(sk, tls_ctx, flags, > > &timeo); > > + if (rc < 0) > > + return rc; > > + > > + pfrag = sk_page_frag(sk); > > + > > + /* TLS_TLS_HEADER_SIZE is not counted as part of the TLS > > record, and > > + * we need to leave room for an authentication tag. > > + */ > > + max_open_record_len = TLS_MAX_PAYLOAD_SIZE + > > + tls_ctx->prepend_size; > > + do { > > + if (tls_do_allocation(sk, ctx, pfrag, > > + tls_ctx->prepend_size)) { > > + rc = sk_stream_wait_memory(sk, &timeo); > > + if (!rc) > > + continue; > > + > > + record = ctx->open_record; > > + if (!record) > > + break; > > +handle_error: > > + if (record_type != TLS_RECORD_TYPE_DATA) { > > + /* avoid sending partial > > + * record with type != > > + * application_data > > + */ > > + size = orig_size; > > + destroy_record(record); > > + ctx->open_record = NULL; > > + } else if (record->len > tls_ctx- > > >prepend_size) { > > + goto last_record; > > + } > > + > > + break; > > + } > > + > > + record = ctx->open_record; > > + copy = min_t(size_t, size, (pfrag->size - pfrag- > > >offset)); > > + copy = min_t(size_t, copy, (max_open_record_len - > > record->len)); > > + > > + if (copy_from_iter_nocache(page_address(pfrag- > > >page) + > > + pfrag->offset, > > + copy, msg_iter) != > > copy) { > > + rc = -EFAULT; > > + goto handle_error; > > + } > > + tls_append_frag(record, pfrag, copy); > > + > > + size -= copy; > > + if (!size) { > > +last_record: > > + tls_push_record_flags = flags; > > + if (more) { > > + tls_ctx->pending_open_record_frags > > = > > + record->num_frags; > > + break; > > + } > > + > > + done = true; > > + } > > + > > + if ((done) || record->len >= max_open_record_len > > || > > + (record->num_frags >= MAX_SKB_FRAGS - 1)) { > > + rc = tls_push_record(sk, > > + tls_ctx, > > + ctx, > > + record, > > + pfrag, > > + tls_push_record_flags > > , > > + record_type); > > + if (rc < 0) > > + break; > > + } > > + } while (!done); > > + > > + if (orig_size - size > 0) > > + rc = orig_size - size; > > + > > + return rc; > > +} > > + > > +int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t > > size) > > +{ > > + unsigned char record_type = TLS_RECORD_TYPE_DATA; > > + int rc = 0; > > + > > + lock_sock(sk); > > + > > + if (unlikely(msg->msg_controllen)) { > > + rc = tls_proccess_cmsg(sk, msg, &record_type); > > + if (rc) > > + goto out; > > + } > > + > > + rc = tls_push_data(sk, &msg->msg_iter, size, > > + msg->msg_flags, record_type); > > + > > +out: > > + release_sock(sk); > > + return rc; > > +} > > + > > +int tls_device_sendpage(struct sock *sk, struct page *page, > > + int offset, size_t size, int flags) > > +{ > > + struct iov_iter msg_iter; > > + struct kvec iov; > > + char *kaddr = kmap(page); > > + int rc = 0; > > + > > + if (flags & MSG_SENDPAGE_NOTLAST) > > + flags |= MSG_MORE; > > + > > + lock_sock(sk); > > + > > + if (flags & MSG_OOB) { > > + rc = -ENOTSUPP; > > + goto out; > > + } > > + > > + iov.iov_base = kaddr + offset; > > + iov.iov_len = size; > > + iov_iter_kvec(&msg_iter, WRITE | ITER_KVEC, &iov, 1, > > size); > > + rc = tls_push_data(sk, &msg_iter, size, > > + flags, TLS_RECORD_TYPE_DATA); > > + kunmap(page); > > + > > +out: > > + release_sock(sk); > > + return rc; > > +} > > + > > +struct tls_record_info *tls_get_record(struct tls_offload_context > > *context, > > + u32 seq, u64 *p_record_sn) > > +{ > > + struct tls_record_info *info; > > + u64 record_sn = context->hint_record_sn; > > + > > + info = context->retransmit_hint; > > + if (!info || > > + before(seq, info->end_seq - info->len)) { > > + /* if retransmit_hint is irrelevant start > > + * from the begging of the list > > + */ > > + info = list_first_entry(&context->records_list, > > + struct tls_record_info, > > list); > > + record_sn = context->unacked_record_sn; > > + } > > + > > + list_for_each_entry_from(info, &context->records_list, > > list) { > > + if (before(seq, info->end_seq)) { > > + if (!context->retransmit_hint || > > + after(info->end_seq, > > + context->retransmit_hint- > > >end_seq)) { > > + context->hint_record_sn = > > record_sn; > > + context->retransmit_hint = info; > > + } > > + *p_record_sn = record_sn; > > + return info; > > + } > > + record_sn++; > > + } > > + > > + return NULL; > > +} > > +EXPORT_SYMBOL(tls_get_record); > > + > > +static int tls_device_push_pending_record(struct sock *sk, int > > flags) > > +{ > > + struct iov_iter msg_iter; > > + > > + iov_iter_kvec(&msg_iter, WRITE | ITER_KVEC, NULL, 0, 0); > > + return tls_push_data(sk, &msg_iter, 0, flags, > > TLS_RECORD_TYPE_DATA); > > +} > > + > > +int tls_set_device_offload(struct sock *sk, struct tls_context > > *ctx) > > +{ > > + u16 nonece_size, tag_size, iv_size, rec_seq_size; > > + struct tls_record_info *start_marker_record; > > + struct tls_offload_context *offload_ctx; > > + struct tls_crypto_info *crypto_info; > > + struct net_device *netdev; > > + char *iv, *rec_seq; > > + struct sk_buff *skb; > > + int rc = -EINVAL; > > + __be64 rcd_sn; > > + > > + if (!ctx) > > + goto out; > > + > > + if (ctx->priv_ctx) { > > + rc = -EEXIST; > > + goto out; > > + } > > + > > + start_marker_record = > > kmalloc(sizeof(*start_marker_record), GFP_KERNEL); > > + if (!start_marker_record) { > > + rc = -ENOMEM; > > + goto out; > > + } > > + > > + offload_ctx = kzalloc(TLS_OFFLOAD_CONTEXT_SIZE, > > GFP_KERNEL); > > + if (!offload_ctx) { > > + rc = -ENOMEM; > > + goto free_marker_record; > > + } > > + > > + crypto_info = &ctx->crypto_send; > > + switch (crypto_info->cipher_type) { > > + case TLS_CIPHER_AES_GCM_128: { > > + nonece_size = TLS_CIPHER_AES_GCM_128_IV_SIZE; > > + tag_size = TLS_CIPHER_AES_GCM_128_TAG_SIZE; > > + iv_size = TLS_CIPHER_AES_GCM_128_IV_SIZE; > > + iv = ((struct tls12_crypto_info_aes_gcm_128 > > *)crypto_info)->iv; > > + rec_seq_size = > > TLS_CIPHER_AES_GCM_128_REC_SEQ_SIZE; > > + rec_seq = > > + ((struct tls12_crypto_info_aes_gcm_128 > > *)crypto_info)->rec_seq; > > + break; > > + } > > + default: > > + rc = -EINVAL; > > + goto free_offload_ctx; > > + } > > + > > + ctx->prepend_size = TLS_HEADER_SIZE + nonece_size; > > + ctx->tag_size = tag_size; > > + ctx->iv_size = iv_size; > > + ctx->iv = kmalloc(iv_size + > > TLS_CIPHER_AES_GCM_128_SALT_SIZE, > > + GFP_KERNEL); > > + if (!ctx->iv) { > > + rc = -ENOMEM; > > + goto free_offload_ctx; > > + } > > + > > + memcpy(ctx->iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, iv, > > iv_size); > > + > > + ctx->rec_seq_size = rec_seq_size; > > + ctx->rec_seq = kmalloc(rec_seq_size, GFP_KERNEL); > > + if (!ctx->rec_seq) { > > + rc = -ENOMEM; > > + goto free_iv; > > + } > > + memcpy(ctx->rec_seq, rec_seq, rec_seq_size); > > + > > + rc = tls_sw_fallback_init(sk, offload_ctx, crypto_info); > > + if (rc) > > + goto free_rec_seq; > > + > > + /* start at rec_seq - 1 to account for the start marker > > record */ > > + memcpy(&rcd_sn, ctx->rec_seq, sizeof(rcd_sn)); > > + offload_ctx->unacked_record_sn = be64_to_cpu(rcd_sn) - 1; > > + > > + start_marker_record->end_seq = tcp_sk(sk)->write_seq; > > + start_marker_record->len = 0; > > + start_marker_record->num_frags = 0; > > + > > + INIT_LIST_HEAD(&offload_ctx->records_list); > > + list_add_tail(&start_marker_record->list, &offload_ctx- > > >records_list); > > + spin_lock_init(&offload_ctx->lock); > > + > > + static_branch_inc(&clean_acked_data_enabled); > > + inet_csk(sk)->icsk_clean_acked = &tls_icsk_clean_acked; > > + ctx->push_pending_record = tls_device_push_pending_record; > > + offload_ctx->sk_destruct = sk->sk_destruct; > > + > > + /* TLS offload is greatly simplified if we don't send > > + * SKBs where only part of the payload needs to be > > encrypted. > > + * So mark the last skb in the write queue as end of > > record. > > + */ > > + skb = tcp_write_queue_tail(sk); > > + if (skb) > > + TCP_SKB_CB(skb)->eor = 1; > > + > > + refcount_set(&ctx->refcount, 1); > > + > > + /* We support starting offload on multiple sockets > > + * concurrently, so we only need a read lock here. > > + * This lock must preceed get_netdev_for_sock to prevent > > races between > > + * NETDEV_DOWN and setsockopt. > > + */ > > + down_read(&device_offload_lock); > > + netdev = get_netdev_for_sock(sk); > > + if (!netdev) { > > + pr_err_ratelimited("%s: netdev not found\n", > > __func__); > > + rc = -EINVAL; > > + goto release_lock; > > + } > > + > > + if (!(netdev->features & NETIF_F_HW_TLS_TX)) { > > + rc = -ENOTSUPP; > > + goto release_netdev; > > + } > > + > > + /* Avoid offloading if the device is down > > + * We don't want to offload new flows after > > + * the NETDEV_DOWN event > > + */ > > + if (!(netdev->flags & IFF_UP)) { > > + rc = -EINVAL; > > + goto release_netdev; > > + } > > + > > + ctx->priv_ctx = offload_ctx; > > + rc = attach_sock_to_netdev(sk, netdev, ctx); > > + if (rc) > > + goto release_netdev; > > + > > + ctx->netdev = netdev; > > + > > + spin_lock_irq(&tls_device_lock); > > + list_add_tail(&ctx->list, &tls_device_list); > > + spin_unlock_irq(&tls_device_lock); > > + > > + sk->sk_validate_xmit_skb = tls_validate_xmit_skb; > > + /* following this assignment tls_is_sk_tx_device_offloaded > > + * will return true and the context might be accessed > > + * by the netdev's xmit function. > > + */ > > + smp_store_release(&sk->sk_destruct, > > + &tls_device_sk_destruct); > > + up_read(&device_offload_lock); > > + goto out; > > + > > +release_netdev: > > + dev_put(netdev); > > +release_lock: > > + up_read(&device_offload_lock); > > + static_branch_dec(&clean_acked_data_enabled); > > + crypto_free_aead(offload_ctx->aead_send); > > +free_rec_seq: > > + kfree(ctx->rec_seq); > > +free_iv: > > + kfree(ctx->iv); > > +free_offload_ctx: > > + kfree(offload_ctx); > > + ctx->priv_ctx = NULL; > > +free_marker_record: > > + kfree(start_marker_record); > > +out: > > + return rc; > > +} > > + > > +static int tls_device_api_check(struct net_device *dev) > > +{ > > + if ((dev->features & NETIF_F_HW_TLS_TX) && !dev- > > >tlsdev_ops) > > + return NOTIFY_BAD; > > You have almost the same check in tls_device_down(). > > Why can't just filter all !(dev->features & NETIF_F_HW_TLS_TX) > devices > in tls_dev_event()? > > I gave the code example in comment to previous patch. > > > + > > + return NOTIFY_DONE; > > +} > > + > > +static int tls_device_down(struct net_device *netdev) > > +{ > > + struct tls_context *ctx, *tmp; > > + struct list_head list; > > + unsigned long flags; > > + > > + if (!(netdev->features & NETIF_F_HW_TLS_TX)) > > + return NOTIFY_DONE; > > + > > + INIT_LIST_HEAD(&list); > > This should just be declarated as: > > LIST_HEAD(list); > > This will save us 2 lines. > > > + > > + /* Request a write lock to block new offload attempts > > + */ > > + down_write(&device_offload_lock); > > + > > + spin_lock_irqsave(&tls_device_lock, flags); > > + list_for_each_entry_safe(ctx, tmp, &tls_device_list, list) > > { > > + if (ctx->netdev != netdev || > > + !refcount_inc_not_zero(&ctx->refcount)) > > + continue; > > + > > + list_move(&ctx->list, &list); > > + } > > + spin_unlock_irqrestore(&tls_device_lock, flags); > > + > > + list_for_each_entry_safe(ctx, tmp, &list, list) { > > + netdev->tlsdev_ops->tls_dev_del(netdev, ctx, > > + TLS_OFFLOAD_CTX_DI > > R_TX); > > + ctx->netdev = NULL; > > + dev_put(netdev); > > + list_del_init(&ctx->list); > > + > > + if (refcount_dec_and_test(&ctx->refcount)) > > + tls_device_free_ctx(ctx); > > + } > > + > > + up_write(&device_offload_lock); > > + > > + flush_work(&tls_device_gc_work); > > + > > + return NOTIFY_DONE; > > +} > > + > > +static int tls_dev_event(struct notifier_block *this, unsigned > > long event, > > + void *ptr) > > +{ > > + struct net_device *dev = netdev_notifier_info_to_dev(ptr); > > + > > + switch (event) { > > + case NETDEV_REGISTER: > > + case NETDEV_FEAT_CHANGE: > > + return tls_device_api_check(dev); > > + case NETDEV_DOWN: > > + return tls_device_down(dev); > > + } > > + return NOTIFY_DONE; > > +} > > + > > +static struct notifier_block tls_dev_notifier = { > > + .notifier_call = tls_dev_event, > > +}; > > + > > +void __init tls_device_init(void) > > +{ > > + register_netdevice_notifier(&tls_dev_notifier); > > +} > > + > > +void __exit tls_device_cleanup(void) > > +{ > > + unregister_netdevice_notifier(&tls_dev_notifier); > > + flush_work(&tls_device_gc_work); > > +} > > diff --git a/net/tls/tls_device_fallback.c > > b/net/tls/tls_device_fallback.c > > new file mode 100644 > > index 000000000000..843c7331cfc4 > > --- /dev/null > > +++ b/net/tls/tls_device_fallback.c > > @@ -0,0 +1,415 @@ > > +/* Copyright (c) 2018, Mellanox Technologies All rights reserved. > > + * > > + * This software is available to you under a choice of one of two > > + * licenses. You may choose to be licensed under the terms of the > > GNU > > + * General Public License (GPL) Version 2, available from the file > > + * COPYING in the main directory of this source tree, or the > > + * OpenIB.org BSD license below: > > + * > > + * Redistribution and use in source and binary forms, with or > > + * without modification, are permitted provided that the > > following > > + * conditions are met: > > + * > > + * - Redistributions of source code must retain the above > > + * copyright notice, this list of conditions and the > > following > > + * disclaimer. > > + * > > + * - Redistributions in binary form must reproduce the above > > + * copyright notice, this list of conditions and the > > following > > + * disclaimer in the documentation and/or other materials > > + * provided with the distribution. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES > > OF > > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT > > HOLDERS > > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN > > AN > > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR > > IN > > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > > THE > > + * SOFTWARE. > > + */ > > + > > +#include <net/tls.h> > > +#include <crypto/aead.h> > > +#include <crypto/scatterwalk.h> > > +#include <net/ip6_checksum.h> > > + > > +static void chain_to_walk(struct scatterlist *sg, struct > > scatter_walk *walk) > > +{ > > + struct scatterlist *src = walk->sg; > > + int diff = walk->offset - src->offset; > > + > > + sg_set_page(sg, sg_page(src), > > + src->length - diff, walk->offset); > > + > > + scatterwalk_crypto_chain(sg, sg_next(src), 0, 2); > > +} > > + > > +static int tls_enc_record(struct aead_request *aead_req, > > + struct crypto_aead *aead, char *aad, > > char *iv, > > + __be64 rcd_sn, struct scatter_walk *in, > > + struct scatter_walk *out, int *in_len) > > +{ > > + struct scatterlist sg_in[3]; > > + struct scatterlist sg_out[3]; > > + unsigned char buf[TLS_HEADER_SIZE + > > TLS_CIPHER_AES_GCM_128_IV_SIZE]; > > Reverse Christmas tree. > > > + u16 len; > > + int rc; > > + > > + len = min_t(int, *in_len, ARRAY_SIZE(buf)); > > + > > + scatterwalk_copychunks(buf, in, len, 0); > > + scatterwalk_copychunks(buf, out, len, 1); > > + > > + *in_len -= len; > > + if (!*in_len) > > + return 0; > > + > > + scatterwalk_pagedone(in, 0, 1); > > + scatterwalk_pagedone(out, 1, 1); > > + > > + len = buf[4] | (buf[3] << 8); > > + len -= TLS_CIPHER_AES_GCM_128_IV_SIZE; > > + > > + tls_make_aad(aad, len - TLS_CIPHER_AES_GCM_128_TAG_SIZE, > > + (char *)&rcd_sn, sizeof(rcd_sn), buf[0]); > > + > > + memcpy(iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, buf + > > TLS_HEADER_SIZE, > > + TLS_CIPHER_AES_GCM_128_IV_SIZE); > > + > > + sg_init_table(sg_in, ARRAY_SIZE(sg_in)); > > + sg_init_table(sg_out, ARRAY_SIZE(sg_out)); > > + sg_set_buf(sg_in, aad, TLS_AAD_SPACE_SIZE); > > + sg_set_buf(sg_out, aad, TLS_AAD_SPACE_SIZE); > > + chain_to_walk(sg_in + 1, in); > > + chain_to_walk(sg_out + 1, out); > > + > > + *in_len -= len; > > + if (*in_len < 0) { > > + *in_len += TLS_CIPHER_AES_GCM_128_TAG_SIZE; > > + if (*in_len < 0) > > + /* the input buffer doesn't contain the entire > > record. > > + * trim len accordingly. The resulting > > authentication tag > > + * will contain garbage. but we don't care as we > > won't > > + * include any of it in the output skb > > + * Note that we assume the output buffer length > > + * is larger then input buffer length + tag size > > + */ > > + len += *in_len; > > + > > + *in_len = 0; > > + } > > + > > + if (*in_len) { > > + scatterwalk_copychunks(NULL, in, len, 2); > > + scatterwalk_pagedone(in, 0, 1); > > + scatterwalk_copychunks(NULL, out, len, 2); > > + scatterwalk_pagedone(out, 1, 1); > > + } > > + > > + len -= TLS_CIPHER_AES_GCM_128_TAG_SIZE; > > + aead_request_set_crypt(aead_req, sg_in, sg_out, len, iv); > > + > > + rc = crypto_aead_encrypt(aead_req); > > + > > + return rc; > > +} > > + > > +static void tls_init_aead_request(struct aead_request *aead_req, > > + struct crypto_aead *aead) > > +{ > > + aead_request_set_tfm(aead_req, aead); > > + aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE); > > +} > > + > > +static struct aead_request *tls_alloc_aead_request(struct > > crypto_aead *aead, > > + gfp_t flags) > > +{ > > + unsigned int req_size = sizeof(struct aead_request) + > > + crypto_aead_reqsize(aead); > > + struct aead_request *aead_req; > > + > > + aead_req = kzalloc(req_size, flags); > > + if (!aead_req) > > + return NULL; > > + > > + tls_init_aead_request(aead_req, aead); > > + return aead_req; > > +} > > + > > +static int tls_enc_records(struct aead_request *aead_req, > > + struct crypto_aead *aead, struct > > scatterlist *sg_in, > > + struct scatterlist *sg_out, char *aad, > > char *iv, > > + u64 rcd_sn, int len) > > +{ > > + struct scatter_walk in; > > + struct scatter_walk out; > > Why do not declare them both in a single line? > > > + int rc; > > + > > + scatterwalk_start(&in, sg_in); > > + scatterwalk_start(&out, sg_out); > > + > > + do { > > + rc = tls_enc_record(aead_req, aead, aad, iv, > > + cpu_to_be64(rcd_sn), &in, > > &out, &len); > > + rcd_sn++; > > + > > + } while (rc == 0 && len); > > + > > + scatterwalk_done(&in, 0, 0); > > + scatterwalk_done(&out, 1, 0); > > + > > + return rc; > > +} > > + > > +static inline void update_chksum(struct sk_buff *skb, int headln) > > +{ > > + /* Can't use icsk->icsk_af_ops->send_check here because > > the ip addresses > > + * might have been changed by NAT. > > + */ > > + > > + const struct ipv6hdr *ipv6h; > > + const struct iphdr *iph; > > + struct tcphdr *th = tcp_hdr(skb); > > + int datalen = skb->len - headln; > > + > > + /* We only changed the payload so if we are using partial > > we don't > > + * need to update anything. > > + */ > > + if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) > > + return; > > + > > + skb->ip_summed = CHECKSUM_PARTIAL; > > + skb->csum_start = skb_transport_header(skb) - skb->head; > > + skb->csum_offset = offsetof(struct tcphdr, check); > > + > > + if (skb->sk->sk_family == AF_INET6) { > > + ipv6h = ipv6_hdr(skb); > > + th->check = ~csum_ipv6_magic(&ipv6h->saddr, > > &ipv6h->daddr, > > + datalen, IPPROTO_TCP, > > 0); > > + } else { > > + iph = ip_hdr(skb); > > + th->check = ~csum_tcpudp_magic(iph->saddr, iph- > > >daddr, datalen, > > + IPPROTO_TCP, 0); > > + } > > +} > > + > > +static void complete_skb(struct sk_buff *nskb, struct sk_buff > > *skb, int headln) > > +{ > > + skb_copy_header(nskb, skb); > > + > > + skb_put(nskb, skb->len); > > + memcpy(nskb->data, skb->data, headln); > > + update_chksum(nskb, headln); > > + > > + nskb->destructor = skb->destructor; > > + nskb->sk = skb->sk; > > + skb->destructor = NULL; > > + skb->sk = NULL; > > + refcount_add(nskb->truesize - skb->truesize, > > + &nskb->sk->sk_wmem_alloc); > > +} > > + > > +/* This function may be called after the user socket is already > > + * closed so make sure we don't use anything freed during > > + * tls_sk_proto_close here > > + */ > > +static struct sk_buff *tls_sw_fallback(struct sock *sk, struct > > sk_buff *skb) > > +{ > > + int tcp_header_size = tcp_hdrlen(skb); > > + int tcp_payload_offset = skb_transport_offset(skb) + > > tcp_header_size; > > + int payload_len = skb->len - tcp_payload_offset; > > + struct tls_context *tls_ctx = tls_get_ctx(sk); > > + struct tls_offload_context *ctx = > > tls_offload_ctx(tls_ctx); > > + int remaining, buf_len, resync_sgs, rc, i = 0; > > + void *buf, *dummy_buf, *iv, *aad; > > + struct scatterlist *sg_in; > > + struct scatterlist sg_out[3]; > > + u32 tcp_seq = ntohl(tcp_hdr(skb)->seq); > > + struct aead_request *aead_req; > > + struct sk_buff *nskb = NULL; > > + struct tls_record_info *record; > > + unsigned long flags; > > + s32 sync_size; > > + u64 rcd_sn; > > I don't remember I've seen a function with so many number of local > variables. > Can we do something to improve the readability of this? > > > + /* worst case is: > > + * MAX_SKB_FRAGS in tls_record_info > > + * MAX_SKB_FRAGS + 1 in SKB head and frags. > > + */ > > + int sg_in_max_elements = 2 * MAX_SKB_FRAGS + 1; > > + > > + if (!payload_len) > > + return skb; > > + > > + sg_in = kmalloc_array(sg_in_max_elements, sizeof(*sg_in), > > GFP_ATOMIC); > > + if (!sg_in) > > + goto free_orig; > > + > > + sg_init_table(sg_in, sg_in_max_elements); > > + sg_init_table(sg_out, ARRAY_SIZE(sg_out)); > > + > > + spin_lock_irqsave(&ctx->lock, flags); > > + record = tls_get_record(ctx, tcp_seq, &rcd_sn); > > + if (!record) { > > + spin_unlock_irqrestore(&ctx->lock, flags); > > + WARN(1, "Record not found for seq %u\n", tcp_seq); > > + goto free_sg; > > + } > > + > > + sync_size = tcp_seq - tls_record_start_seq(record); > > + if (sync_size < 0) { > > + int is_start_marker = > > tls_record_is_start_marker(record); > > + > > + spin_unlock_irqrestore(&ctx->lock, flags); > > + if (!is_start_marker) > > + /* This should only occur if the relevant record > > was > > + * already acked. In that case it should be ok > > + * to drop the packet and avoid retransmission. > > + * > > + * There is a corner case where the packet > > contains > > + * both an acked and a non-acked record. > > + * We currently don't handle that case and rely > > + * on TCP to retranmit a packet that doesn't > > contain > > + * already acked payload. > > + */ > > + goto free_orig; > > + > > + if (payload_len > -sync_size) { > > + WARN(1, "Fallback of partially offloaded > > packets is not supported\n"); > > + goto free_sg; > > + } else { > > + return skb; > > + } > > + } > > + > > + remaining = sync_size; > > + while (remaining > 0) { > > + skb_frag_t *frag = &record->frags[i]; > > + > > + __skb_frag_ref(frag); > > + sg_set_page(sg_in + i, skb_frag_page(frag), > > + skb_frag_size(frag), frag- > > >page_offset); > > + > > + remaining -= skb_frag_size(frag); > > + > > + if (remaining < 0) > > + sg_in[i].length += remaining; > > + > > + i++; > > + } > > + spin_unlock_irqrestore(&ctx->lock, flags); > > + resync_sgs = i; > > + > > + aead_req = tls_alloc_aead_request(ctx->aead_send, > > GFP_ATOMIC); > > + if (!aead_req) > > + goto put_sg; > > + > > + buf_len = TLS_CIPHER_AES_GCM_128_SALT_SIZE + > > + TLS_CIPHER_AES_GCM_128_IV_SIZE + > > + TLS_AAD_SPACE_SIZE + > > + sync_size + > > + tls_ctx->tag_size; > > + buf = kmalloc(buf_len, GFP_ATOMIC); > > + if (!buf) > > + goto free_req; > > + > > + nskb = alloc_skb(skb_headroom(skb) + skb->len, > > GFP_ATOMIC); > > + if (!nskb) > > + goto free_buf; > > + > > + skb_reserve(nskb, skb_headroom(skb)); > > + > > + iv = buf; > > + > > + memcpy(iv, tls_ctx->crypto_send_aes_gcm_128.salt, > > + TLS_CIPHER_AES_GCM_128_SALT_SIZE); > > + aad = buf + TLS_CIPHER_AES_GCM_128_SALT_SIZE + > > + TLS_CIPHER_AES_GCM_128_IV_SIZE; > > + dummy_buf = aad + TLS_AAD_SPACE_SIZE; > > + > > + sg_set_buf(&sg_out[0], dummy_buf, sync_size); > > + sg_set_buf(&sg_out[1], nskb->data + tcp_payload_offset, > > + payload_len); > > + /* Add room for authentication tag produced by crypto */ > > + dummy_buf += sync_size; > > + sg_set_buf(&sg_out[2], dummy_buf, tls_ctx->tag_size); > > + rc = skb_to_sgvec(skb, &sg_in[i], tcp_payload_offset, > > + payload_len); > > + if (rc < 0) > > + goto free_nskb; > > + > > + rc = tls_enc_records(aead_req, ctx->aead_send, sg_in, > > sg_out, aad, iv, > > + rcd_sn, sync_size + payload_len); > > + if (rc < 0) > > + goto free_nskb; > > + > > + complete_skb(nskb, skb, tcp_payload_offset); > > + > > + /* validate_xmit_skb_list assumes that if the skb wasn't > > segmented > > + * nskb->prev will point to the skb itself > > + */ > > + nskb->prev = nskb; > > +free_buf: > > + kfree(buf); > > +free_req: > > + kfree(aead_req); > > +put_sg: > > + for (i = 0; i < resync_sgs; i++) > > + put_page(sg_page(&sg_in[i])); > > +free_sg: > > + kfree(sg_in); > > +free_orig: > > + kfree_skb(skb); > > + return nskb; > > + > > +free_nskb: > > + kfree_skb(nskb); > > + nskb = NULL; > > + goto free_buf; > > +} > > + > > +struct sk_buff *tls_validate_xmit_skb(struct sock *sk, > > + struct net_device *dev, > > + struct sk_buff *skb) > > +{ > > + if (dev == tls_get_ctx(sk)->netdev) > > + return skb; > > + > > + return tls_sw_fallback(sk, skb); > > +} > > + > > +int tls_sw_fallback_init(struct sock *sk, > > + struct tls_offload_context *offload_ctx, > > + struct tls_crypto_info *crypto_info) > > +{ > > + int rc; > > + const u8 *key; > > Reverse Christmas tree. > > > + > > + offload_ctx->aead_send = > > + crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC); > > + if (IS_ERR(offload_ctx->aead_send)) { > > + rc = PTR_ERR(offload_ctx->aead_send); > > + pr_err_ratelimited("crypto_alloc_aead failed > > rc=%d\n", rc); > > + offload_ctx->aead_send = NULL; > > + goto err_out; > > + } > > + > > + key = ((struct tls12_crypto_info_aes_gcm_128 > > *)crypto_info)->key; > > + > > + rc = crypto_aead_setkey(offload_ctx->aead_send, key, > > + TLS_CIPHER_AES_GCM_128_KEY_SIZE); > > + if (rc) > > + goto free_aead; > > + > > + rc = crypto_aead_setauthsize(offload_ctx->aead_send, > > + TLS_CIPHER_AES_GCM_128_TAG_SI > > ZE); > > + if (rc) > > + goto free_aead; > > + > > + return 0; > > +free_aead: > > + crypto_free_aead(offload_ctx->aead_send); > > +err_out: > > + return rc; > > +} > > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c > > index d824d548447e..e0dface33017 100644 > > --- a/net/tls/tls_main.c > > +++ b/net/tls/tls_main.c > > @@ -54,6 +54,9 @@ enum { > > enum { > > TLS_BASE_TX, > > TLS_SW_TX, > > +#ifdef CONFIG_TLS_DEVICE > > + TLS_HW_TX, > > +#endif > > TLS_NUM_CONFIG, > > }; > > > > @@ -416,11 +419,19 @@ static int do_tls_setsockopt_tx(struct sock > > *sk, char __user *optval, > > goto err_crypto_info; > > } > > > > - /* currently SW is default, we will have ethtool in future > > */ > > - rc = tls_set_sw_offload(sk, ctx); > > - tx_conf = TLS_SW_TX; > > - if (rc) > > - goto err_crypto_info; > > +#ifdef CONFIG_TLS_DEVICE > > + rc = tls_set_device_offload(sk, ctx); > > + tx_conf = TLS_HW_TX; > > + if (rc) { > > +#else > > + { > > +#endif > > + /* if HW offload fails fallback to SW */ > > + rc = tls_set_sw_offload(sk, ctx); > > + tx_conf = TLS_SW_TX; > > + if (rc) > > + goto err_crypto_info; > > + } > > > > ctx->tx_conf = tx_conf; > > update_sk_prot(sk, ctx); > > @@ -473,6 +484,12 @@ static void build_protos(struct proto *prot, > > struct proto *base) > > prot[TLS_SW_TX] = prot[TLS_BASE_TX]; > > prot[TLS_SW_TX].sendmsg = tls_sw_sendmsg; > > prot[TLS_SW_TX].sendpage = tls_sw_sendpage; > > + > > +#ifdef CONFIG_TLS_DEVICE > > + prot[TLS_HW_TX] = prot[TLS_SW_TX]; > > + prot[TLS_HW_TX].sendmsg = > > tls_device_sendmsg; > > + prot[TLS_HW_TX].sendpage = tls_device_sendpage; > > +#endif > > } > > > > static int tls_init(struct sock *sk) > > @@ -531,6 +548,9 @@ static int __init tls_register(void) > > { > > build_protos(tls_prots[TLSV4], &tcp_prot); > > > > +#ifdef CONFIG_TLS_DEVICE > > + tls_device_init(); > > +#endif > > tcp_register_ulp(&tcp_tls_ulp_ops); > > > > return 0; > > @@ -539,6 +559,9 @@ static int __init tls_register(void) > > static void __exit tls_unregister(void) > > { > > tcp_unregister_ulp(&tcp_tls_ulp_ops); > > +#ifdef CONFIG_TLS_DEVICE > > + tls_device_cleanup(); > > +#endif > > } > > > > module_init(tls_register); > > Thanks, > Kirill