linux-kernel - Re: [RFC v5 3/5] virtio_ring: add packed ring support

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]

Date:   Tue, 29 May 2018 14:16:01 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Tiwei Bie <tiwei.bie@...el.com>
Cc:     mst@...hat.com, virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        wexu@...hat.com, jfreimann@...hat.com
Subject: Re: [RFC v5 3/5] virtio_ring: add packed ring support



On 2018年05月29日 13:11, Tiwei Bie wrote:
> On Tue, May 29, 2018 at 11:18:57AM +0800, Jason Wang wrote:
>> On 2018年05月22日 16:16, Tiwei Bie wrote:
> [...]
>>> +static void detach_buf_packed(struct vring_virtqueue *vq,
>>> +			      unsigned int id, void **ctx)
>>> +{
>>> +	struct vring_desc_state_packed *state;
>>> +	struct vring_packed_desc *desc;
>>> +	unsigned int i;
>>> +	int *next;
>>> +
>>> +	/* Clear data ptr. */
>>> +	vq->desc_state_packed[id].data = NULL;
>>> +
>>> +	next = &id;
>>> +	for (i = 0; i < vq->desc_state_packed[id].num; i++) {
>>> +		state = &vq->desc_state_packed[*next];
>>> +		vring_unmap_state_packed(vq, state);
>>> +		next = &vq->desc_state_packed[*next].next;
>> Have a short discussion with Michael. It looks like the only thing we care
>> is DMA address and length here.
> The length tracked by desc_state_packed[] is also necessary
> when INDIRECT is used and the buffers are writable.
>
>> So a thought is to avoid using desc_state_packed() is vring_use_dma_api() is
>> false? Probably need another id allocator or just use desc_state_packed for
>> free id list.
> I think it's a good suggestion. Thanks!
>
> I won't track the addr/len/flags for non-indirect descs
> when vring_use_dma_api() returns false.
>
>>> +	}
>>> +
>>> +	vq->vq.num_free += vq->desc_state_packed[id].num;
>>> +	*next = vq->free_head;
>> Using pointer seems not elegant and unnecessary here. You can just track
>> next in integer I think?
> It's possible to use integer, but will need one more var
> to track `prev` (desc_state_packed[prev].next == next in
> this case).

Yes, please do this.

>
>>> +	vq->free_head = id;
>>> +
>>> +	if (vq->indirect) {
>>> +		u32 len;
>>> +
>>> +		/* Free the indirect table, if any, now that it's unmapped. */
>>> +		desc = vq->desc_state_packed[id].indir_desc;
>>> +		if (!desc)
>>> +			return;
>>> +
>>> +		len = vq->desc_state_packed[id].len;
>>> +		for (i = 0; i < len / sizeof(struct vring_packed_desc); i++)
>>> +			vring_unmap_desc_packed(vq, &desc[i]);
>>> +
>>> +		kfree(desc);
>>> +		vq->desc_state_packed[id].indir_desc = NULL;
>>> +	} else if (ctx) {
>>> +		*ctx = vq->desc_state_packed[id].indir_desc;
>>> +	}
>>>    }
>>>    static inline bool more_used_packed(const struct vring_virtqueue *vq)
>>>    {
>>> -	return false;
>>> +	u16 last_used, flags;
>>> +	u8 avail, used;
>>> +
>>> +	last_used = vq->last_used_idx;
>>> +	flags = virtio16_to_cpu(vq->vq.vdev,
>>> +				vq->vring_packed.desc[last_used].flags);
>>> +	avail = !!(flags & VRING_DESC_F_AVAIL(1));
>>> +	used = !!(flags & VRING_DESC_F_USED(1));
>>> +
>>> +	return avail == used && used == vq->used_wrap_counter;
>> Spec does not check avail == used? So there's probably a bug in either of
>> the two places.
>>
>> In what condition that avail != used but used == vq_used_wrap_counter? A
>> buggy device or device set the two bits in two writes?
> Currently, spec doesn't forbid devices to set the status
> bits as: avail != used but used == vq_used_wrap_counter.
> So I think driver cannot assume this won't happen.

Right, but I could not find a situation that device need to something 
like this. We should either forbid this in the spec or change the 
example code in the spec.

Let's see how Michael think about this.

Thanks