netdev - Re: [PATCH v9 crypto 01/12] tls: tls_device struct to register TLS drivers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d2a47f73-dd58-4ac0-76c7-77acfd115c46@chelsio.com>
Date:   Wed, 7 Mar 2018 16:51:51 +0530
From:   Atul Gupta <atul.gupta@...lsio.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     davejwatson@...com, davem@...emloft.net,
        herbert@...dor.apana.org.au, linux-crypto@...r.kernel.org,
        netdev@...r.kernel.org, ganeshgr@...lsio.com
Subject: Re: [PATCH v9 crypto 01/12] tls: tls_device struct to register TLS
 drivers



On 3/7/2018 3:46 PM, Sabrina Dubroca wrote:
> Hello Atul,
>
> One quick note before you start replying: please fix your email
> client, as you've been told before. Quoting has to add a quoting
> marker (the '>' character) at the beginning of the line, otherwise
> it's impossible to separate your reply from the email you're quoting.
>
> 2018-03-06, 21:06:20 +0530, Atul Gupta wrote:
>> tls_device structure to register Inline TLS drivers
>> with net/tls
>>
>> Signed-off-by: Atul Gupta <atul.gupta@...lsio.com>
>> ---
>>  include/net/tls.h | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/include/net/tls.h b/include/net/tls.h
>> index 4913430..9bfb91f 100644
>> --- a/include/net/tls.h
>> +++ b/include/net/tls.h
>> @@ -55,6 +55,28 @@
>>  #define TLS_RECORD_TYPE_DATA		0x17
>>  
>>  #define TLS_AAD_SPACE_SIZE		13
>> +#define TLS_DEVICE_NAME_MAX		32
> Why 32 characters?
This is considered to accommodate registering device name, should it be bigger?
>
>
>> +enum {
>> +	TLS_BASE_TX,
>> +	TLS_SW_TX,
>> +	TLS_FULL_HW, /* TLS record processed Inline */
>> +	TLS_NUM_CONFIG,
>> +};
>> +
>> +extern struct proto tls_prots[TLS_NUM_CONFIG];
> Don't break bisection. The code has to compile after every
> patch. These are already defined in net/tls/tls_main.c.
Will take care
>
>> +struct tls_device {
>> +	char name[TLS_DEVICE_NAME_MAX];
> I could find only one use of it, in chtls_register_dev. Is this
> actually needed?
help to identify Inline TLS drivers registered with net/tls
>
>> +	struct list_head dev_list;
>> +
>> +	/* netdev present in registered inline tls driver */
>> +	int (*netdev)(struct tls_device *device,
>> +		      struct net_device *netdev);
> I was trying to figure out what this did, because the name is really
> not explicit, and the comment doesn't make sense, but noticed it's
> never actually called.
Was used Initially, removed in last few cleanup [thanks for pointing]
>
>> +	int (*feature)(struct tls_device *device);
>> +	int (*hash)(struct tls_device *device, struct sock *sk);
>> +	void (*unhash)(struct tls_device *device, struct sock *sk);
> I think you should add a kerneldoc comment, like all the ndo_*
> methods have.
Will take care
>
>> +};
>>  
>>  struct tls_sw_context {
>>  	struct crypto_aead *aead_send;
>> @@ -115,6 +137,8 @@ struct tls_context {
>>  	int  (*getsockopt)(struct sock *sk, int level,
>>  			   int optname, char __user *optval,
>>  			   int __user *optlen);
>> +	int (*hash)(struct sock *sk);
>> +	void (*unhash)(struct sock *sk);
>>  };
>>  
>>  int wait_on_pending_writer(struct sock *sk, long *timeo);
>> @@ -256,5 +280,7 @@ static inline struct tls_offload_context *tls_offload_ctx(
>>  
>>  int tls_proccess_cmsg(struct sock *sk, struct msghdr *msg,
>>  		      unsigned char *record_type);
>> +void tls_register_device(struct tls_device *device);
>> +void tls_unregister_device(struct tls_device *device);
> Prototype without implementation, please don't do that.  This happens
> because you split your patchset so that each patch has all the changes
> for exactly one file.
will have declaration and definition together
>