New OP-TEE SHM design. #29

lorc · 2016-12-13T19:16:36Z

Those patches are kernel driver part of dynamic shared memory in OP-TEE (refer to OP-TEE/optee_os#1232). Two of those patches are authored by @jenswi-linaro, I just tidied it a bit and rebased on current master.

This patches provide new ioctl to userpace along with support in tee_shm and op-tee kernel part. This ioctl allows userspace process to share own memory with OP-TEE.

Another part - is OP-TEE specific SHM pool that uses kernel memory. With this approach we don't rely on OP-TEE predefined pool.

etienne-lms

late feedback.
few minors and an error case to crosscheck.
very nice.

etienne-lms · 2017-01-11T13:45:01Z

drivers/tee/tee_core.c

+	data.id = -1;
+
+	shm = tee_shm_register(ctx, data.addr, data.length,
+			       TEE_SHM_DMA_BUF|TEE_SHM_USER_MAPPED);


minor: spaces between |

etienne-lms · 2017-01-11T13:48:50Z

drivers/tee/tee_core.c

+	if (data.flags)
+		return -EINVAL;
+
+	data.id = -1;


minor: why this? (same question applies to tee_ioctl_shm_alloc() above.

etienne-lms · 2017-01-11T13:56:27Z

drivers/tee/tee_shm.c

+		mutex_lock(&teedev->mutex);
+		list_add_tail(&shm->link, &ctx->list_shm);
+		mutex_unlock(&teedev->mutex);
+


minor: empty line

etienne-lms · 2017-01-11T14:09:41Z

drivers/tee/tee_shm.c

+	void *ret;
+	int rc;
+	int num_pages;
+	size_t n;


minor: n could move to loop.

etienne-lms · 2017-01-11T14:10:04Z

drivers/tee/tee_shm.c

+				 size_t length, u32 flags)
+{
+	struct tee_device *teedev = ctx->teedev;
+	u32 req_flags = TEE_SHM_DMA_BUF|TEE_SHM_USER_MAPPED;


minor: spaces between |

etienne-lms · 2017-01-11T16:13:51Z

drivers/tee/optee/call.c

+		mp->attr |= OPTEE_MSG_ATTR_FRAGMENT;
+		mp++;
+		/* Check if we filled whole page with addresses */
+		if (((uintptr_t)(mp + 1) & ~PAGE_MASK) == 0) {


i don't catch this.
should not be ((uintptr_t)mp & PAGE_MASK) != (uintptr_t)(mp + 1) & PAGE_MASK) ?

lorc · 2017-01-24T20:07:40Z

Addressed @etienne-lms comments, rebased on actual branch.

jenswi-linaro

Why is an own shared memory pool needed? It does more or less the same thing as the pool in the subsystem.

jenswi-linaro · 2017-01-25T12:58:35Z

include/linux/tee_drv.h

+/**
+ * tee_shm_is_registered() - Check if shared memory object in registered in TEE
+ * @shm:	Shared memory handle
+ * @returns true if object is registed in TEE


s/registed/registered/

jenswi-linaro · 2017-01-25T13:03:55Z

drivers/tee/optee/call.c

@@ -433,3 +437,126 @@ void optee_disable_shm_cache(struct optee *optee)
 	}
 	optee_cq_wait_final(&optee->call_queue, &w);
 }
+
+size_t optee_correct_params_cnt(size_t num_params)


I find this name confusing. What about using optee_round_up_params_count() instead?

lorc · 2017-03-10T19:05:17Z

Rebased, addressed comments. Also added new patch (for privileged calls) and simplified RPC memory allocation.

@jenswi-linaro, own shared memory pool needed because I want to use kernel memory for command buffers.

jenswi-linaro · 2017-03-13T18:00:51Z

@lorc, kmalloc() should be able to supply the memory needed for command data and vmalloc() for the larger allocation, or am I missing something?

lorc · 2017-03-13T18:12:26Z

@jenswi-linaro, you are right. Actually, "new shared memory pool" is not a right name for it. But it is the term used in tee code. It is more like an allocator. New "pool" uses kmalloc() to allocate command buffers. And it uses OP-TEE shared memory region to allocate dma_buf regions, so TEE_IOC_SHM_ALLOC will allocate memory from that plain old 2M buffer. It was left for compatibility because new clients will not use TEE_IOC_SHM_ALLOC.

lorc · 2017-06-02T22:06:23Z

Hi. I just rebased this on actual branch and also added delta patch with new ABI and new RPC cleanup approach.

jenswi-linaro · 2017-06-05T11:12:31Z

drivers/tee/optee/call.c

+}
+
+/* Number of user pages + number of pages to hold list of user pages  */
+#define GET_ARRAY_SIZE(num_entries) \


Declare this as a static function instead.

jenswi-linaro · 2017-06-05T11:13:18Z

drivers/tee/optee/call.c

+
+	msg_arg->cmd = OPTEE_MSG_CMD_REGISTER_SHM;
+	msg_arg->params->attr = OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
+		OPTEE_MSG_ATTR_NONCONTIG;


Align with beginning of OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT above.

jenswi-linaro · 2017-06-05T11:14:11Z

drivers/tee/optee/call.c

+	msg_arg->params->u.tmem.shm_ref = (unsigned long)shm;
+	msg_arg->params->u.tmem.size = tee_shm_get_size(shm);
+	msg_arg->params->u.tmem.buf_ptr = virt_to_phys(pages_array) |
+		tee_shm_get_page_offset(shm);


Align with virt_to_phys() above.

jenswi-linaro · 2017-06-05T11:15:42Z

drivers/tee/optee/core.c

+		case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT:
+		case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT:
+			p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT +
+				attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT;


jenswi-linaro · 2017-06-05T11:16:06Z

drivers/tee/optee/core.c

+			/* Check that the memref is covered by the shm object */
+			if (p->u.memref.size) {
+				size_t o = p->u.memref.shm_offs +
+					p->u.memref.size;


jenswi-linaro · 2017-06-05T11:21:39Z

drivers/tee/optee/rpc.c

 {
 	phys_addr_t pa;
 	struct tee_shm *shm;
 	size_t sz;
 	size_t n;

+	BUG_ON(!call_ctx || call_ctx->pages_array || call_ctx->num_entries);


This shouldn't be needed

jenswi-linaro · 2017-06-05T11:23:01Z

drivers/tee/optee/rpc.c

+		call_ctx->num_entries = page_num;
+
+		arg->params[0].attr =  OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
+			OPTEE_MSG_ATTR_NONCONTIG;


jenswi-linaro · 2017-06-05T11:23:38Z

drivers/tee/optee/rpc.c

+		arg->params[0].attr =  OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
+			OPTEE_MSG_ATTR_NONCONTIG;
+		arg->params[0].u.tmem.buf_ptr = virt_to_phys(pages_array)
+			+ tee_shm_get_page_offset(shm);


+ should be at the end of the line above
alignment

jenswi-linaro · 2017-06-05T11:32:31Z

drivers/tee/optee/rpc.c

@@ -375,16 +430,19 @@ static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee,
 		handle_rpc_func_cmd_wait(arg);
 		break;
 	case OPTEE_MSG_RPC_CMD_SHM_ALLOC:
-		handle_rpc_func_cmd_shm_alloc(ctx, arg);
+		free_page_array(call_ctx);


Contrary to what I've said before, it should be enough to call this function here (and of course from optee_rpc_finalize_call()) as not much is gained by calling it for all the other RPCs.

jenswi-linaro · 2017-06-05T11:41:11Z

drivers/tee/tee_shm.c

+ *
+ * This function is not thread safe and have no reference counter
+ */
+void *tee_shm_vmap(struct tee_shm *shm)


Why do we need this function?

lorc · 2017-06-06T20:49:44Z

Addressed comments (and also checkpatch issues). Removed function tee_shm_vmap() with friends (it was used in prev. ABI version).

jenswi-linaro · 2017-06-07T09:48:32Z

drivers/tee/optee/call.c

@@ -480,21 +480,22 @@ void optee_fill_pages_list(u64 *dst, struct page **pages, size_t num_pages)
 }

 /* Number of user pages + number of pages to hold list of user pages  */
-#define GET_ARRAY_SIZE(num_entries) \
-	sizeof(u64) * (num_entries + (sizeof(u64) * num_entries) / PAGE_SIZE)
+size_t get_pages_array_size(size_t num_entries)


lorc · 2017-06-07T14:06:37Z

added v3 delta patch, where get_pages_array_size() was made static function

jenswi-linaro

I wouldn't mind if this pull request had all the commits squashed and cleaned up either in this pull request or in a new pull request replacing this.

I'd like this to be possible to upstream more or less as is when we finally merge it.

jenswi-linaro · 2017-07-11T12:24:47Z

drivers/tee/optee/optee_private.h

 int optee_from_msg_param(struct tee_param *params, size_t num_params,
 			 const struct optee_msg_param *msg_params);
 int optee_to_msg_param(struct optee_msg_param *msg_params, size_t num_params,
 		       const struct tee_param *params);

+void *optee_allocate_pages_array(size_t num_entries);


This should return a u64 * instead as that's what it's used for and num_entries also says how many u64 the returned array should be able to hold.

jenswi-linaro · 2017-07-11T12:40:21Z

drivers/tee/optee/core.c

-	pool = tee_shm_pool_alloc_res_mem(&priv_info, &dmabuf_info);
+	/* If OP-TEE can work with unregistered SHM, we will use own pool */
+	if (sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
+		pool = optee_shm_get_pool(&dmabuf_info);


We don't need this new "own pool" just because OP-TEE can work with unregistered shared memory.
I'd rather remove it from this pull request.

So you suggest to move it to an another PR? Because we need some mechanism to allocate command buffers.

Aha, sorry I didn't realize that the already present pool doesn't allocate command buffers correctly. Please keep the "own pool".

In the meantime I'll try to figure out how this can work together with the already present pool instead of replacing it with partially duplicated code.

I've created a pull request (lorc#1) into this branch making pool creation more flexible.

jenswi-linaro · 2017-07-11T12:54:26Z

drivers/tee/optee/optee_private.h

@@ -137,7 +149,7 @@ int optee_supp_recv(struct tee_context *ctx, u32 *func, u32 *num_params,
 int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
 		    struct tee_param *param);

-u32 optee_do_call_with_arg(struct tee_context *ctx, phys_addr_t parg);
+u32 optee_do_call_with_arg(struct tee_device *teedev, phys_addr_t parg);


I regret these teedev instead of ``ctxandoptee` instead of `ctx` changes. They mostly add a bunch of unrelated changes that doesn't help much. Wouldn't you agree?

Yeah, I agree. I'll look on how to get it back. AFAIR, there was some reason for that change.

lorc · 2017-07-21T21:57:13Z

@jenswi-linaro, thank you for your pull request. I used your patches for tee as is, but I had to squash your patches for shm_pool into mine ones, because I changed patches layout. As you can see, now there are a lot more patches. I tried to separate different parts of this big feature into separate patches.

But now I'm facing the problem. As you suggested, I reverted that part for your patch that changed ctx to teedev. Problem is that I can't unregister shared buffer because it's context is being erased on close:

static void teedev_close_context(struct tee_context *ctx)
{
	struct tee_shm *shm;

	ctx->teedev->desc->ops->release(ctx);
	mutex_lock(&ctx->teedev->mutex);
	list_for_each_entry(shm, &ctx->list_shm, link)
		shm->ctx = NULL; <====== Right there
	mutex_unlock(&ctx->teedev->mutex);
	tee_device_put(ctx->teedev);
	kfree(ctx);
}

So, it all is fails with this backtrace:

[    4.712751] [<ffff000008774f80>] tee_shm_alloc+0x10/0x20
[    4.712860] [<ffff000008775ba4>] get_msg_arg+0x34/0xd0
[    4.712962] [<ffff000008776610>] optee_shm_unregister+0x28/0xb0
[    4.713070] [<ffff0000087744d0>] tee_shm_do_release+0xd8/0xf8
[    4.713174] [<ffff000008774558>] tee_shm_release+0x68/0x80
[    4.713275] [<ffff000008774580>] tee_shm_op_release+0x10/0x18
[    4.713382] [<ffff00000853ab58>] dma_buf_release+0x60/0x1a0
[    4.713508] [<ffff0000081daad4>] __fput+0x8c/0x1d0

Because tee_shm_alloc() receives null context.

I think, this is a reason, why you used teedev instead of ctx in the first place.

Looks like we need to add reference counter to tee_ctx to make it live until last shm will be freed. Is this right approach?

jenswi-linaro · 2017-07-24T12:10:08Z

Aha, yes I think using a reference counter is more clear.

jbech-linaro · 2017-09-19T10:38:40Z

Hi @lorc , I think we should merge this on our linaro-swg/linux/optee branch, but then as Jens suggested, next thing is to send it to the kernel mailinglist, since that is where we like this to end up in the end. @etienne-lms @jenswi-linaro and other comments left?

lorc · 2017-09-19T10:44:31Z

Hi @jbech-linaro, sure, I'll submit this patch series to LKML. I just want to be sure that you guys are okay with the patches.

jenswi-linaro · 2017-09-19T10:51:20Z

Merge Enable dynamic SHM support OP-TEE/optee_os#1631
Merge New OP-TEE SHM design. #29 (with a "[NEW SHM]" prefix in the title on all commits to make it easy to identify the commits which are part of this and should be upstreamed)
Merge tee_client_api: register user memory OP-TEE/optee_client#72

Once that's done it's time for @lorc to send the linux patches to LKML and take another round of review there. During the review process we'll keep the testable setup in our development branch (optee) which we'll update with delta patches as needed.

jforissier · 2017-09-19T11:06:56Z

+1 with what @jenswi-linaro said. Let me add action items:

Merge Enable dynamic SHM support OP-TEE/optee_os#1631

Action @lorc : rebase onto current master, squash some commits together, apply R-b tags. See also my comment about rephrasing some commit subjects (but feel free to ignore them if it turns out I did not understand correctly!).
I'll merge the PR as soon as this is done.

Merge New OP-TEE SHM design. #29 (with a "[NEW SHM]" prefix in the title on all commits to make it easy to identify the commits which are part of this and should be upstreamed)

Action @lorc : add the "[NEW SHM]" prefix.
@jenswi-linaro : should A-b or R-b be added at this point?

Merge tee_client_api: register user memory OP-TEE/optee_client#72

Action @lorc : address pending comments, rebase onto master (resolve conflicts).
Then we will gather the A-b/R-b and merge.

This is a cool feature and I'd really like to see it merged soon! So let's finalize this stuff :)
Thanks.

jenswi-linaro · 2017-09-19T11:23:23Z

Action @lorc : add the "[NEW SHM]" prefix.
@jenswi-linaro : should A-b or R-b be added at this point?

No, I'd rather take that on LKML. In the end I expect that I'll send the outcome of the review as a separate pull request to arm-soc with my S-o-B added to all commits.

etienne-lms

I had a new review round to the patches. Please find the minor stylish findings...

Comment of commit "tee: shm: add accessors for buffer size and page offset"
~~This~~These two functions

Comment of commit "tee: shm: add page accessor functions "
... function to returns list ...

Comment of commit "tee: optee: add page list manipulation functions"
~~This~~These functions ...

Comment of commit: "tee: optee: add shared buffer registration functions"

... can use ~~this~~these functions to **(un)**register ...
Please note , that while (remove coma) those functions ~~was~~were added to optee code ...

Comment of commit "tee: optee: add optee-specific shared pool implementation"

This is a simple pool that uses kernel page allocator. This pool can be used in case if OP-TEE supports dynamic shared memory.

etienne-lms · 2017-09-19T11:09:22Z

include/linux/tee_drv.h

+ */
+static inline ssize_t tee_shm_get_size(struct tee_shm *shm)
+{
+	return shm->size;


sh->size is a size_t, this returns a ssize_t. Likely to not be an issue, yet tee_shm_get_size() could simply return a size_t.

etienne-lms · 2017-09-19T11:12:54Z

include/linux/tee_drv.h

@@ -404,6 +404,20 @@ static inline ssize_t tee_shm_get_size(struct tee_shm *shm)
 }

 /**
+ * tee_shm_get_pages() - Get list of pages that hold shared buffer


minor: s/hold/holds/

etienne-lms · 2017-09-19T11:25:55Z

drivers/tee/optee/optee_msg.h

+ * Every entry in buffer should point to a 4k page beginning (12 least
+ * significant bits must be equal to zero).
+ *
+ * 12 least significant of optee_msg_param.u.tmem.buf_ptr should hold page


minor: " * 12 least significant bits of optee_msg_param.u.tmem.buf_ptr should hold page
offset of the user buffer."

etienne-lms · 2017-09-19T11:43:24Z

drivers/tee/optee/call.c

+
+static size_t get_pages_array_size(size_t num_entries)
+{
+	/* Number of user pages + number of pages to hold list of user pages  */


Minor: dual space at comment end

etienne-lms · 2017-09-19T12:01:09Z

drivers/tee/optee/call.c

+int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
+		       struct page **pages, size_t num_pages)
+{
+	struct tee_shm *shm_arg = NULL;


minor: useless init (although this change is overridden by a later change)

etienne-lms · 2017-09-19T12:07:05Z

drivers/tee/optee/core.c

+			if (p->u.memref.size) {
+				size_t o = p->u.memref.shm_offs +
+					   p->u.memref.size;
+				if (o > tee_shm_get_size(shm))


minor: add empty line above

etienne-lms · 2017-09-19T12:17:20Z

drivers/tee/optee/core.c

+			goto err_memunmap;
+		priv_mgr = rc;
+	} else {
+		size_t sz = OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE;


etienne-lms · 2017-09-19T12:21:50Z

include/linux/tee_drv.h

@@ -42,11 +43,17 @@ struct tee_shm_pool;
 * @teedev:	pointer to this drivers struct tee_device
 * @list_shm:	List of shared memory object owned by this context
 * @data:	driver specific context data, managed by the driver
+ * @registered: reference counter for this structure


s/registered/refcount/

Makes creation of shm pools more flexible by adding new more primitive functions to allocate a shm pool. This makes it easier to add driver specific shm pool management. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Added new ioctl to allow users register own buffers as a shared memory. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

These two function will be needed for shared memory registration in OP-TEE Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

In order to register a shared buffer in TEE, we need accessor function that return list of pages for that buffer. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

There were changes in REE<->OP-TEE ABI recently. Now ABI allows us to pass non-contiguous memory buffers as list of pages to OP-TEE. This can be achieved by using new parameter attribute OPTEE_MSG_ATTR_NONCONTIG. OP-TEE also is able to use all non-secure RAM for shared buffers. This new capability is enabled with OPTEE_SMC_SEC_CAP_DYNAMIC_SHM flag. This patch adds necessary definitions to the protocol definition files at Linux side. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

These functions will be used to pass information about shared buffers to OP-TEE. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

This change adds ops for shm_(un)register functions in tee interface. Client application can use these functions to (un)register an own shared buffer in OP-TEE address space. This allows zero copy data sharing between Normal and Secure Worlds. Please note that while those functions were added to optee code, it does not report to userspace that those functions are available. OP-TEE code does not set TEE_GEN_CAP_REG_MEM flag. This flag will be enabled only after all other features of dynamic shared memory will be implemented in subsequent patches. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

Now, when client applications can register own shared buffers in OP-TEE, we need to extend ABI for parameter passing to/from OP-TEE. So, if OP-TEE core detects that parameter belongs to registered shared memory, it will use corresponding parameter attribute. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

With latest changes to OP-TEE we can use any buffers as a shared memory. Thus, it is possible for supplicant to provide part of own memory when OP-TEE asks to allocate a shared buffer. This patch adds support for such feature into RPC handling code. Now when OP-TEE asks supplicant to allocate shared buffer, supplicant can use TEE_IOC_SHM_REGISTER to provide such buffer. RPC handler is aware of this, so it will pass list of allocated pages to OP-TEE. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

Those capabilities will be used in subsequent patches. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

This is simple pool that uses kernel page allocator. This pool can be used in case OP-TEE supports dynamic shared memory. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

Previous patches added various features that are needed for dynamic SHM. Dynamic SHM allows Normal World to share any buffers with OP-TEE. While original design suggested to use pre-allocated region (usually of 1M to 2M of size), this new approach allows to use all non-secure RAM for command buffers, RPC allocations and TA parameters. This patch checks capability OPTEE_SMC_SEC_CAP_DYNAMIC_SHM. If it was set by OP-TEE, then kernel part of OP-TEE will use kernel page allocator to allocate command buffers. Also it will set TEE_GEN_CAP_REG_MEM capability to tell userspace that it supports shared memory registration. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

We need to ensure that tee_context is present until last shared buffer will be freed. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

Now, when struct tee_shm is defined in public header, we can inline small getter functions. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

lorc · 2017-09-20T19:56:54Z

Thank you for review, Etienne. I addressed your comments right in appropriate patches.
Also I added [NEW SHM] prefix to whole series.

lorc · 2017-09-21T16:10:49Z

If there are no other comments on this PR, I'm going to post this patches to LKML later today.

lorc · 2017-09-21T20:28:57Z

Ah, this is not so easy, as I expected :) It will take some time to resolve conflicts with master branch.
@jenswi-linaro, there are your S-o-b tag on your patches, that I'm going to submit as a part of the series.
But tee: add register user memory was reworked by me. Do you wish me to preserve your S-o-b tag?

(Honestly, it is the first time, I'm submitting сo-authored patches, I don't quite what is the correct procedure).

jenswi-linaro · 2017-09-22T05:39:15Z

@lorc, I'm OK either way with tee: add register user memory, I guess I would have kept all S-o-bs.

syzbot found the following crash on: HEAD commit: d9bd94c Add linux-next specific files for 20180801 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=1001189c400000 kernel config: https://syzkaller.appspot.com/x/.config?x=cc8964ea4d04518c dashboard link: https://syzkaller.appspot.com/bug?extid=c966a82db0b14aa37e81 compiler: gcc (GCC) 8.0.1 20180413 (experimental) Unfortunately, I don't have any reproducer for this crash yet. IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+c966a82db0b14aa37e81@syzkaller.appspotmail.com loop7: rw=12288, want=8200, limit=20 netlink: 65342 bytes leftover after parsing attributes in process `syz-executor4'. openvswitch: netlink: Message has 8 unknown bytes. kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] SMP KASAN CPU: 1 PID: 7615 Comm: syz-executor7 Not tainted 4.18.0-rc7-next-20180801+ #29 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:__read_once_size include/linux/compiler.h:188 [inline] RIP: 0010:compound_head include/linux/page-flags.h:142 [inline] RIP: 0010:PageLocked include/linux/page-flags.h:272 [inline] RIP: 0010:f2fs_put_page fs/f2fs/f2fs.h:2011 [inline] RIP: 0010:validate_checkpoint+0x66d/0xec0 fs/f2fs/checkpoint.c:835 Code: e8 58 05 7f fe 4c 8d 6b 80 4d 8d 74 24 08 48 b8 00 00 00 00 00 fc ff df 4c 89 ea 48 c1 ea 03 c6 04 02 00 4c 89 f2 48 c1 ea 03 <80> 3c 02 00 0f 85 f4 06 00 00 4c 89 ea 4d 8b 7c 24 08 48 b8 00 00 RSP: 0018:ffff8801937cebe8 EFLAGS: 00010246 RAX: dffffc0000000000 RBX: ffff8801937cef30 RCX: ffffc90006035000 RDX: 0000000000000000 RSI: ffffffff82fd9658 RDI: 0000000000000005 RBP: ffff8801937cef58 R08: ffff8801ab254700 R09: fffff94000d9e026 R10: fffff94000d9e026 R11: ffffea0006cf0137 R12: fffffffffffffffb R13: ffff8801937ceeb0 R14: 0000000000000003 R15: ffff880193419b40 FS: 00007f36a61d5700(0000) GS:ffff8801db100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fc04ff93000 CR3: 00000001d0562000 CR4: 00000000001426e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: f2fs_get_valid_checkpoint+0x436/0x1ec0 fs/f2fs/checkpoint.c:860 f2fs_fill_super+0x2d42/0x8110 fs/f2fs/super.c:2883 mount_bdev+0x314/0x3e0 fs/super.c:1344 f2fs_mount+0x3c/0x50 fs/f2fs/super.c:3133 legacy_get_tree+0x131/0x460 fs/fs_context.c:729 vfs_get_tree+0x1cb/0x5c0 fs/super.c:1743 do_new_mount fs/namespace.c:2603 [inline] do_mount+0x6f2/0x1e20 fs/namespace.c:2927 ksys_mount+0x12d/0x140 fs/namespace.c:3143 __do_sys_mount fs/namespace.c:3157 [inline] __se_sys_mount fs/namespace.c:3154 [inline] __x64_sys_mount+0xbe/0x150 fs/namespace.c:3154 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x45943a Code: b8 a6 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 bd 8a fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 9a 8a fb ff c3 66 0f 1f 84 00 00 00 00 00 RSP: 002b:00007f36a61d4a88 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5 RAX: ffffffffffffffda RBX: 00007f36a61d4b30 RCX: 000000000045943a RDX: 00007f36a61d4ad0 RSI: 0000000020000100 RDI: 00007f36a61d4af0 RBP: 0000000020000100 R08: 00007f36a61d4b30 R09: 00007f36a61d4ad0 R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000013 R13: 0000000000000000 R14: 00000000004c8ea0 R15: 0000000000000000 Modules linked in: Dumping ftrace buffer: (ftrace buffer empty) ---[ end trace bd8550c129352286 ]--- RIP: 0010:__read_once_size include/linux/compiler.h:188 [inline] RIP: 0010:compound_head include/linux/page-flags.h:142 [inline] RIP: 0010:PageLocked include/linux/page-flags.h:272 [inline] RIP: 0010:f2fs_put_page fs/f2fs/f2fs.h:2011 [inline] RIP: 0010:validate_checkpoint+0x66d/0xec0 fs/f2fs/checkpoint.c:835 Code: e8 58 05 7f fe 4c 8d 6b 80 4d 8d 74 24 08 48 b8 00 00 00 00 00 fc ff df 4c 89 ea 48 c1 ea 03 c6 04 02 00 4c 89 f2 48 c1 ea 03 <80> 3c 02 00 0f 85 f4 06 00 00 4c 89 ea 4d 8b 7c 24 08 48 b8 00 00 RSP: 0018:ffff8801937cebe8 EFLAGS: 00010246 RAX: dffffc0000000000 RBX: ffff8801937cef30 RCX: ffffc90006035000 RDX: 0000000000000000 RSI: ffffffff82fd9658 RDI: 0000000000000005 netlink: 65342 bytes leftover after parsing attributes in process `syz-executor4'. RBP: ffff8801937cef58 R08: ffff8801ab254700 R09: fffff94000d9e026 openvswitch: netlink: Message has 8 unknown bytes. R10: fffff94000d9e026 R11: ffffea0006cf0137 R12: fffffffffffffffb R13: ffff8801937ceeb0 R14: 0000000000000003 R15: ffff880193419b40 FS: 00007f36a61d5700(0000) GS:ffff8801db100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fc04ff93000 CR3: 00000001d0562000 CR4: 00000000001426e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 In validate_checkpoint(), if we failed to call get_checkpoint_version(), we will pass returned invalid page pointer into f2fs_put_page, cause accessing invalid memory, this patch tries to handle error path correctly to fix this issue. Signed-off-by: Chao Yu <yuchao0@huawei.com> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

commit d3f07c0 upstream. syzbot found the following crash on: HEAD commit: d9bd94c Add linux-next specific files for 20180801 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=1001189c400000 kernel config: https://syzkaller.appspot.com/x/.config?x=cc8964ea4d04518c dashboard link: https://syzkaller.appspot.com/bug?extid=c966a82db0b14aa37e81 compiler: gcc (GCC) 8.0.1 20180413 (experimental) Unfortunately, I don't have any reproducer for this crash yet. IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+c966a82db0b14aa37e81@syzkaller.appspotmail.com loop7: rw=12288, want=8200, limit=20 netlink: 65342 bytes leftover after parsing attributes in process `syz-executor4'. openvswitch: netlink: Message has 8 unknown bytes. kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] SMP KASAN CPU: 1 PID: 7615 Comm: syz-executor7 Not tainted 4.18.0-rc7-next-20180801+ #29 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:__read_once_size include/linux/compiler.h:188 [inline] RIP: 0010:compound_head include/linux/page-flags.h:142 [inline] RIP: 0010:PageLocked include/linux/page-flags.h:272 [inline] RIP: 0010:f2fs_put_page fs/f2fs/f2fs.h:2011 [inline] RIP: 0010:validate_checkpoint+0x66d/0xec0 fs/f2fs/checkpoint.c:835 Code: e8 58 05 7f fe 4c 8d 6b 80 4d 8d 74 24 08 48 b8 00 00 00 00 00 fc ff df 4c 89 ea 48 c1 ea 03 c6 04 02 00 4c 89 f2 48 c1 ea 03 <80> 3c 02 00 0f 85 f4 06 00 00 4c 89 ea 4d 8b 7c 24 08 48 b8 00 00 RSP: 0018:ffff8801937cebe8 EFLAGS: 00010246 RAX: dffffc0000000000 RBX: ffff8801937cef30 RCX: ffffc90006035000 RDX: 0000000000000000 RSI: ffffffff82fd9658 RDI: 0000000000000005 RBP: ffff8801937cef58 R08: ffff8801ab254700 R09: fffff94000d9e026 R10: fffff94000d9e026 R11: ffffea0006cf0137 R12: fffffffffffffffb R13: ffff8801937ceeb0 R14: 0000000000000003 R15: ffff880193419b40 FS: 00007f36a61d5700(0000) GS:ffff8801db100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fc04ff93000 CR3: 00000001d0562000 CR4: 00000000001426e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: f2fs_get_valid_checkpoint+0x436/0x1ec0 fs/f2fs/checkpoint.c:860 f2fs_fill_super+0x2d42/0x8110 fs/f2fs/super.c:2883 mount_bdev+0x314/0x3e0 fs/super.c:1344 f2fs_mount+0x3c/0x50 fs/f2fs/super.c:3133 legacy_get_tree+0x131/0x460 fs/fs_context.c:729 vfs_get_tree+0x1cb/0x5c0 fs/super.c:1743 do_new_mount fs/namespace.c:2603 [inline] do_mount+0x6f2/0x1e20 fs/namespace.c:2927 ksys_mount+0x12d/0x140 fs/namespace.c:3143 __do_sys_mount fs/namespace.c:3157 [inline] __se_sys_mount fs/namespace.c:3154 [inline] __x64_sys_mount+0xbe/0x150 fs/namespace.c:3154 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x45943a Code: b8 a6 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 bd 8a fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 9a 8a fb ff c3 66 0f 1f 84 00 00 00 00 00 RSP: 002b:00007f36a61d4a88 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5 RAX: ffffffffffffffda RBX: 00007f36a61d4b30 RCX: 000000000045943a RDX: 00007f36a61d4ad0 RSI: 0000000020000100 RDI: 00007f36a61d4af0 RBP: 0000000020000100 R08: 00007f36a61d4b30 R09: 00007f36a61d4ad0 R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000013 R13: 0000000000000000 R14: 00000000004c8ea0 R15: 0000000000000000 Modules linked in: Dumping ftrace buffer: (ftrace buffer empty) ---[ end trace bd8550c129352286 ]--- RIP: 0010:__read_once_size include/linux/compiler.h:188 [inline] RIP: 0010:compound_head include/linux/page-flags.h:142 [inline] RIP: 0010:PageLocked include/linux/page-flags.h:272 [inline] RIP: 0010:f2fs_put_page fs/f2fs/f2fs.h:2011 [inline] RIP: 0010:validate_checkpoint+0x66d/0xec0 fs/f2fs/checkpoint.c:835 Code: e8 58 05 7f fe 4c 8d 6b 80 4d 8d 74 24 08 48 b8 00 00 00 00 00 fc ff df 4c 89 ea 48 c1 ea 03 c6 04 02 00 4c 89 f2 48 c1 ea 03 <80> 3c 02 00 0f 85 f4 06 00 00 4c 89 ea 4d 8b 7c 24 08 48 b8 00 00 RSP: 0018:ffff8801937cebe8 EFLAGS: 00010246 RAX: dffffc0000000000 RBX: ffff8801937cef30 RCX: ffffc90006035000 RDX: 0000000000000000 RSI: ffffffff82fd9658 RDI: 0000000000000005 netlink: 65342 bytes leftover after parsing attributes in process `syz-executor4'. RBP: ffff8801937cef58 R08: ffff8801ab254700 R09: fffff94000d9e026 openvswitch: netlink: Message has 8 unknown bytes. R10: fffff94000d9e026 R11: ffffea0006cf0137 R12: fffffffffffffffb R13: ffff8801937ceeb0 R14: 0000000000000003 R15: ffff880193419b40 FS: 00007f36a61d5700(0000) GS:ffff8801db100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fc04ff93000 CR3: 00000001d0562000 CR4: 00000000001426e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 In validate_checkpoint(), if we failed to call get_checkpoint_version(), we will pass returned invalid page pointer into f2fs_put_page, cause accessing invalid memory, this patch tries to handle error path correctly to fix this issue. Signed-off-by: Chao Yu <yuchao0@huawei.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

When more than a single SCMI device are present in the system, the creation of the notification workqueue with the WQ_SYSFS flag will lead to the following sysfs duplicate node warning: sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scmi_notify' CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.9.0-gdf4dd84a3f7d linaro-swg#29 Hardware name: Broadcom STB (Flattened Device Tree) Workqueue: events deferred_probe_work_func Backtrace: show_stack + 0x20/0x24 dump_stack + 0xbc/0xe0 sysfs_warn_dup + 0x70/0x80 sysfs_create_dir_ns + 0x15c/0x1a4 kobject_add_internal + 0x140/0x4d0 kobject_add + 0xc8/0x138 device_add + 0x1dc/0xc20 device_register + 0x24/0x28 workqueue_sysfs_register + 0xe4/0x1f0 alloc_workqueue + 0x448/0x6ac scmi_notification_init + 0x78/0x1dc scmi_probe + 0x268/0x4fc platform_drv_probe + 0x70/0xc8 really_probe + 0x184/0x728 driver_probe_device + 0xa4/0x278 __device_attach_driver + 0xe8/0x148 bus_for_each_drv + 0x108/0x158 __device_attach + 0x190/0x234 device_initial_probe + 0x1c/0x20 bus_probe_device + 0xdc/0xec deferred_probe_work_func + 0xd4/0x11c process_one_work + 0x420/0x8f0 worker_thread + 0x4fc/0x91c kthread + 0x21c/0x22c ret_from_fork + 0x14/0x20 kobject_add_internal failed for scmi_notify with -EEXIST, don't try to register things with the same name in the same directory. arm-scmi brcm_scmi@1: SCMI Notifications - Initialization Failed. arm-scmi brcm_scmi@1: SCMI Notifications NOT available. arm-scmi brcm_scmi@1: SCMI Protocol v1.0 'brcm-scmi:' Firmware version 0x1 Fix this by using dev_name(handle->dev) which guarantees that the name is unique and this also helps correlate which notification workqueue corresponds to which SCMI device instance. Link: https://lore.kernel.org/r/20201014021737.287340-1-f.fainelli@gmail.com Fixes: bd31b24 ("firmware: arm_scmi: Add notification dispatch and delivery") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> [sudeep.holla: trimmed backtrace to remove all unwanted hexcodes and timestamps] Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> [etienne: picked b9ceca6] Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

lorc force-pushed the new_shm_rel branch from d0b4520 to 0357574 Compare December 13, 2016 19:27

jforissier force-pushed the optee branch from 0adbb24 to 1bdaf42 Compare January 9, 2017 16:31

etienne-lms reviewed Jan 11, 2017

View reviewed changes

lorc force-pushed the new_shm_rel branch 3 times, most recently from fc40e05 to 7ce2cde Compare January 24, 2017 20:07

jenswi-linaro reviewed Jan 25, 2017

View reviewed changes

lorc force-pushed the new_shm_rel branch from 7ce2cde to 751f66f Compare March 10, 2017 19:03

etienne-lms mentioned this pull request Mar 13, 2017

Is there a way to pass fixed physical memory region to TA as a parameter (shm_mem)? OP-TEE/optee_os#1409

Closed

lorc force-pushed the new_shm_rel branch 2 times, most recently from a31f438 to ab827c6 Compare April 5, 2017 17:22

lorc mentioned this pull request Apr 5, 2017

[RFC] Dynamic shared memory implementation. OP-TEE/optee_os#1232

Closed

lorc mentioned this pull request May 31, 2017

add post-RPC cleanup framework #45

Closed

lorc force-pushed the new_shm_rel branch from ab827c6 to d33bebb Compare June 2, 2017 22:04

jenswi-linaro reviewed Jun 5, 2017

View reviewed changes

jenswi-linaro reviewed Jun 7, 2017

View reviewed changes

lorc mentioned this pull request Jul 6, 2017

Enable dynamic SHM support OP-TEE/optee_os#1631

Merged

jenswi-linaro reviewed Jul 11, 2017

View reviewed changes

lorc force-pushed the new_shm_rel branch 2 times, most recently from 201a254 to 6b9034a Compare July 21, 2017 18:27

etienne-lms reviewed Sep 19, 2017

View reviewed changes

jenswi-linaro and others added 14 commits September 20, 2017 20:07

[NEW SHM]tee: flexible shared memory pool creation

d315dcb

Makes creation of shm pools more flexible by adding new more primitive functions to allocate a shm pool. This makes it easier to add driver specific shm pool management. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

[NEW SHM]tee: add register user memory

8581fd4

Added new ioctl to allow users register own buffers as a shared memory. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

[NEW SHM]tee: shm: add accessors for buffer size and page offset

576f5a5

These two function will be needed for shared memory registration in OP-TEE Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

[NEW SHM]tee: shm: add page accessor functions

2b45115

In order to register a shared buffer in TEE, we need accessor function that return list of pages for that buffer. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

[NEW SHM]tee: optee: add page list manipulation functions

cebe229

These functions will be used to pass information about shared buffers to OP-TEE. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

[NEW SHM]tee: optee: store OP-TEE capabilities in private data

9ead85c

Those capabilities will be used in subsequent patches. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

[NEW SHM]tee: optee: add optee-specific shared pool implementation

7660114

This is simple pool that uses kernel page allocator. This pool can be used in case OP-TEE supports dynamic shared memory. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

[NEW SHM]tee: use reference counting for tee_context

a5ec6de

We need to ensure that tee_context is present until last shared buffer will be freed. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

[NEW SHM]tee: shm: inline tee_shm getter functions

7d65aa0

Now, when struct tee_shm is defined in public header, we can inline small getter functions. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>

lorc force-pushed the new_shm_rel branch from cab7fa7 to 7d65aa0 Compare September 20, 2017 19:53

jforissier merged commit 0c6f7dd into linaro-swg:optee Oct 5, 2017

lorc deleted the new_shm_rel branch October 9, 2017 12:14

New OP-TEE SHM design. #29

New OP-TEE SHM design. #29

Conversation

lorc commented Dec 13, 2016

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorc commented Jan 24, 2017

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorc commented Mar 10, 2017

jenswi-linaro commented Mar 13, 2017

lorc commented Mar 13, 2017

lorc commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorc commented Jun 6, 2017

Choose a reason for hiding this comment

lorc commented Jun 7, 2017

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorc commented Jul 21, 2017

jenswi-linaro commented Jul 24, 2017

jbech-linaro commented Sep 19, 2017

lorc commented Sep 19, 2017

jenswi-linaro commented Sep 19, 2017

jforissier commented Sep 19, 2017

jenswi-linaro commented Sep 19, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorc commented Sep 20, 2017

lorc commented Sep 21, 2017

lorc commented Sep 21, 2017

jenswi-linaro commented Sep 22, 2017