new ioctl in TEE driver to register shared memory from a file descriptor (SWG-186)

[etienne-lms]

This is a proposal for implementing the ability for a TA to safely handle memory buffer arguments related to secure buffers. See SWG-186.

This proposal defines a new ioctl in the TEE linux driver TEE_IOC_SHM_REGISTER_FD for userland get create a TEE shared memory object from a provided dmabuf file descriptor provided from userland.

This PR is related to changes in the OP-TEE client: OP-TEE/optee_client#59.

[jenswi-linaro]

I think it would be better to have a struct like:

struct tee_shm_dmabuf_ref {
     struct tee_shm;
     struct dma_buf *dmabuf;
     struct dma_buf_attachment *attach;
     struct sg_table *sgt
};

and use container_of to go from a pointer to a struct tee_shm to a pointer to a struct tee_shm_dmabuf_ref.

[etienne-lms]

ok

[jenswi-linaro]

So without TEE_SHM_MAPPED this is always what we would get? It would feel a bit safer to have something more than the absence of a flag to indicate that there's a struct dmabuf_ref following.

[etienne-lms]

What about a flag TEE_EXT_DMA_BUF, different from TEE_SHM_DMA_BUF, explicitly stating that the shm object relates to both an external dmabuf reference (the one the user registered) and a local dmabuf reference (the one create by tee_shm.c to provide to userland a file descriptor for our shm object).

[jenswi-linaro]

TEE_SHM_EXT_DMA_BUF sounds good.

[jenswi-linaro]

ref && is missing, wouldn't it be better to surround these three ifs with a if (ref) {...?

[etienne-lms]

thanks

[jenswi-linaro]

Missed this in the update?

[etienne-lms]

Completly missed. My apoligies.

[jenswi-linaro]

By calling tee_shm_release() the actual shm object is released before the dmabuf reference is released.

[etienne-lms]

The comment is wrong: we must release the original dmabuf reference only once our local shm object is released.

[jenswi-linaro]

I see, but why is that needed?

[etienne-lms]

In case of an registered external dmabuf, releasing the shm object requires releasing an sgt and an attachment on the dmabuf before releasing the dmabuf itself, as done in the proposed tee_shm_release().

[jenswi-linaro]

shm->dmabuf is not the same dmabuf as the one registered or am I missing something?

[etienne-lms]

hmmm:
I stored in shm->dmabuf the dmabuf reference used to track our shm object.
The user registered dmabuf reference is stored in another structure (together with the sgt resources).

This allows TEE driver to handle shm->dmabuf as the reference for the shm (which TEE driver is interested in): it can be used to map, close, .... the shm object.

The original dmabuf reference the user registered through a fd, is kept aside and specifically handled only at shm object creation and a shm object destruction.

[jenswi-linaro]

That means that calling dma_buf_put() would be OK here.

[etienne-lms]

yes, you are right. (i should not be surprised)
tee_shm_free() should call dma_buf_put().... in case the buffer was registered from kernel land and needs to be freed from kernel land.

thanks, i will fix.

[jenswi-linaro]

It seems a bit pointless to NULL initialize this variable.

[etienne-lms]

done

[jenswi-linaro]

Missed this in the update?

[etienne-lms]

done

[etienne-lms]

Completly missed. My apoligies.

[etienne-lms]

update on-going restoring legacy ioc ids.

[jenswi-linaro]

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

[etienne-lms]

squashed, tag applied.