netdev - Re: [PATCH v9 crypto 08/12] chtls: Key program

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: <ce4ad5b8-e20b-5e75-eb30-af10da87eb7f@chelsio.com>
Date:   Thu, 8 Mar 2018 16:48:54 +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 08/12] chtls: Key program



On 3/7/2018 8:35 PM, Sabrina Dubroca wrote:
> 2018-03-06, 21:09:27 +0530, Atul Gupta wrote:
> [snip]
>> +static int chtls_set_tcb_field(struct sock *sk, u16 word, u64 mask, u64 val)
>> +{
>> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
>> +	struct sk_buff *skb;
>> +	struct cpl_set_tcb_field *req;
>> +	struct ulptx_idata *sc;
>> +	unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
>> +	unsigned int credits_needed = DIV_ROUND_UP(wrlen, 16);
>> +
>> +	skb = alloc_skb(wrlen, GFP_ATOMIC);
> I haven't followed the whole call chain, but does this really need to
> be an atomic allocation?
this is used to send control info to HW, will look at it again for atomic usage
> Should this skb be charged to the socket's memory accounting?
>
>> +	if (!skb)
>> +		return -ENOMEM;
>> +
>> +	__set_tcb_field(sk, skb, word, mask, val, 0, 1);
>> +	set_queue(skb, (csk->txq_idx << 1) | CPL_PRIORITY_DATA, sk);
>> +	csk->wr_credits -= credits_needed;
>> +	csk->wr_unacked += credits_needed;
>> +	enqueue_wr(csk, skb);
>> +	cxgb4_ofld_send(csk->egress_dev, skb);
> Should the return value be checked?
Yes, good to check
>
>> +	return 0;
>> +}
>
> [snip]
>> +static void *chtls_alloc_mem(unsigned long size)
>> +{
>> +	void *p = kmalloc(size, GFP_KERNEL);
>> +
>> +	if (!p)
>> +		p = vmalloc(size);
>> +	if (p)
>> +		memset(p, 0, size);
>> +	return p;
>> +}
> I think you replace this with kvzalloc().
taken care, thanks
>
>> +static void chtls_free_mem(void *addr)
>> +{
>> +	unsigned long p = (unsigned long)addr;
>> +
>> +	if (p >= VMALLOC_START && p < VMALLOC_END)
>> +		vfree(addr);
>> +	else
>> +		kfree(addr);
>> +}
> Use kvfree().
done
>
>> +/* TLS Key bitmap processing */
>> +int chtls_init_kmap(struct chtls_dev *cdev, struct cxgb4_lld_info *lldi)
>> +{
>> +	unsigned int num_key_ctx, bsize;
>> +
>> +	num_key_ctx = (lldi->vr->key.size / TLS_KEY_CONTEXT_SZ);
>> +	bsize = BITS_TO_LONGS(num_key_ctx);
>> +
>> +	cdev->kmap.size = num_key_ctx;
>> +	cdev->kmap.available = bsize;
>> +	cdev->kmap.addr = chtls_alloc_mem(sizeof(*cdev->kmap.addr) *
>> +					  bsize);
>> +	if (!cdev->kmap.addr)
>> +		return -1;
> The return value of this function is never checked.
return not required, will revisit.
>
>> +
>> +	cdev->kmap.start = lldi->vr->key.start;
>> +	spin_lock_init(&cdev->kmap.lock);
>> +	return 0;
>> +}
>> +
>> +void chtls_free_kmap(struct chtls_dev *cdev)
>> +{
>> +	if (cdev->kmap.addr)
>> +		chtls_free_mem(cdev->kmap.addr);
>> +}
> I think this wrapper function is not necessary.
removed
>
>> +static int get_new_keyid(struct chtls_sock *csk, u32 optname)
>> +{
>> +	struct chtls_dev *cdev = csk->cdev;
>> +	struct chtls_hws *hws = &csk->tlshws;
>> +	struct net_device *dev = csk->egress_dev;
>> +	struct adapter *adap = netdev2adap(dev);
>> +	int keyid;
>> +
>> +	spin_lock_bh(&cdev->kmap.lock);
>> +	keyid = find_first_zero_bit(cdev->kmap.addr, cdev->kmap.size);
>> +	if (keyid < cdev->kmap.size) {
>> +		__set_bit(keyid, cdev->kmap.addr);
>> +		if (optname == TLS_RX)
> TLS_RX only gets defined in patch 11, so the only reason you're not
> breaking the build is because all these new files aren't getting
> compiled until patch 12.
yes
>
>> +			hws->rxkey = keyid;
>> +		else
>> +			hws->txkey = keyid;
>> +		atomic_inc(&adap->chcr_stats.tls_key);
>> +	} else {
>> +		keyid = -1;
>> +	}
>> +	spin_unlock_bh(&cdev->kmap.lock);
>> +	return keyid;
>> +}
>> +
> [snip]
>> +int chtls_setkey(struct chtls_sock *csk, u32 keylen, u32 optname)
>> +{
> ...
>> +	skb = alloc_skb(len, GFP_KERNEL);
>> +	if (!skb)
>> +		return -ENOMEM;
> This return code is also ignored by its caller.  Please review the
> whole patchset for that problem.
will do, thanks
>
> Same question as before, does this need to be accounted?
>
>
>> +	/* ulptx command */
>> +	kwr->req.cmd = cpu_to_be32(ULPTX_CMD_V(ULP_TX_MEM_WRITE) |
>> +			    T5_ULP_MEMIO_ORDER_V(1) |
>> +			    T5_ULP_MEMIO_IMM_V(1));
>> +	kwr->req.len16 = cpu_to_be32((csk->tid << 8) |
>> +			      DIV_ROUND_UP(len - sizeof(kwr->wr), 16));
>> +	kwr->req.dlen = cpu_to_be32(ULP_MEMIO_DATA_LEN_V(klen >> 5));
>> +	kwr->req.lock_addr = cpu_to_be32(ULP_MEMIO_ADDR_V(keyid_to_addr
>> +					(cdev->kmap.start, keyid)));
> Breaking this line in that way makes it really hard to read for
> humans.
I will clean this
Thanks
Atul
>