New issue
Advanced search Search tips

Issue 804428 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

vgem function signature mismatch in 3.14 kernel.

Project Member Reported by evgreen@chromium.org, Jan 22 2018

Issue description

While attempting to make the kernel compile without warnings I noticed a warning about assignment of pointer from incompatible type:

/mnt/host/source/src/third_party/kernel/v3.14/drivers/gpu/drm/vgem/vgem_drv.c:365:2: warning: initialization from incompatible pointer type
  .gem_prime_import_sg_table = vgem_gem_prime_import_sg_table,

Indeed, the function signature for the type did not match vgem's implementation of the function.

The way vgem_gem_prime_import_sg_table is implemented, the mismatch should cause a panic if executed (since the function is dereferencing a size as if it were a pointer). 

vgem_test has a test to exercise this code path (importing buffers from a foreign card). Unfortunately execution never gets into vgem_gem_prime_import_sg_table because:
1) vgem_test always opens /dev/dri/card0, which in many (all?) cases is vgem itself.
2) There is short circuit logic in drm_gem_prime_import that says "if I'm importing from my own card, exit early".
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 26 2018

Labels: merge-merged-chromeos-3.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/17b78064fb34ff422a95019f87c235822ddff64e

commit 17b78064fb34ff422a95019f87c235822ddff64e
Author: Evan Green <evgreen@chromium.org>
Date: Fri Jan 26 03:23:32 2018

CHROMIUM: drm/vgem: Function signature mismatch.

This change fixes a function pointer signature mismatch introduced
during backporting.

include/drm/drmP.h defines the signature of gem_prime_import_sg_table as

struct drm_gem_object *
(*gem_prime_import_sg_table)(
	struct drm_device *dev,
	size_t size,
	struct sg_table	*sgt);

but it was defined by driver/gpu/drm/vgem/vgem_dma_buf.c as

struct drm_gem_object *
vgem_gem_prime_import_sg_table(struct drm_device *dev,
			       struct dma_buf_attachment *attach,
			       struct sg_table *sg)

The function treats the size parameter as a dma_bug_attachment *,
dereferencing it twice and passing that to an object_init function.

BUG= chromium:804428 
TEST=vgem_test with recent modifications

Change-Id: I18378ac09d27fc49e0484da72641a3bde43bed4d
Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/879396
Commit-Ready: Evan Green <evgreen@google.com>
Tested-by: Evan Green <evgreen@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>

[modify] https://crrev.com/17b78064fb34ff422a95019f87c235822ddff64e/drivers/gpu/drm/vgem/vgem_drv.h
[modify] https://crrev.com/17b78064fb34ff422a95019f87c235822ddff64e/drivers/gpu/drm/vgem/vgem_dma_buf.c

Cc: za...@chromium.org gurcheta...@chromium.org
Labels: -Pri-3 M-65 Merge-Request-65 Pri-2
Seems wise to merge this back to M-65
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/drm-tests/+/50a23fecf8d351bc0adc5d725d52307cecfd7263

commit 50a23fecf8d351bc0adc5d725d52307cecfd7263
Author: Evan Green <evgreen@chromium.org>
Date: Sat Jan 27 02:17:19 2018

drm-tests: vgem_test: Exercise import_sg_table

The import_foreign part of vgem test is meant to exercise the import and
export of buffers between vgem and other cards. The test however always
opens /dev/dri/card0, which in many cases is vgem itself. There is
short-circuit logic in drm_gem_prime_import which notices if the buffer
is being imported/exported to the same device, and exits early if so.

This change opens a device that has resources vgem doesn't have, to
ensure that a non-vgem card is used to exercise the full import code
path in the kernel. With this change, the kernel bug in 3.14
chromium:804428 can be observed.

BUG= chromium:804428 
TEST=Added and observed printk in vgem_gem_prime_import_sg_table and
drm_gem_prime_import.
CQ-DEPEND=CL:879396

Signed-off-by: Evan Green <evgreen@chromium.org>
Change-Id: Id014cdbae34802783943d4675dd1ff71d7359382
Reviewed-on: https://chromium-review.googlesource.com/879398
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Evan Green <evgreen@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>

[modify] https://crrev.com/50a23fecf8d351bc0adc5d725d52307cecfd7263/vgem_test.c

Project Member

Comment 4 by sheriffbot@chromium.org, Jan 27 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 29 2018

Labels: merge-merged-release-R65-10323.B-chromeos-3.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/abf3ddd5738f325063ed129208813db819aa2e5e

commit abf3ddd5738f325063ed129208813db819aa2e5e
Author: Evan Green <evgreen@chromium.org>
Date: Mon Jan 29 17:33:57 2018

CHROMIUM: drm/vgem: Function signature mismatch.

This change fixes a function pointer signature mismatch introduced
during backporting.

include/drm/drmP.h defines the signature of gem_prime_import_sg_table as

struct drm_gem_object *
(*gem_prime_import_sg_table)(
	struct drm_device *dev,
	size_t size,
	struct sg_table	*sgt);

but it was defined by driver/gpu/drm/vgem/vgem_dma_buf.c as

struct drm_gem_object *
vgem_gem_prime_import_sg_table(struct drm_device *dev,
			       struct dma_buf_attachment *attach,
			       struct sg_table *sg)

The function treats the size parameter as a dma_bug_attachment *,
dereferencing it twice and passing that to an object_init function.

BUG= chromium:804428 
TEST=vgem_test with recent modifications

Change-Id: I18378ac09d27fc49e0484da72641a3bde43bed4d
Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/879396
Commit-Ready: Evan Green <evgreen@google.com>
Tested-by: Evan Green <evgreen@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
(cherry picked from commit 17b78064fb34ff422a95019f87c235822ddff64e)
Reviewed-on: https://chromium-review.googlesource.com/891398
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

[modify] https://crrev.com/abf3ddd5738f325063ed129208813db819aa2e5e/drivers/gpu/drm/vgem/vgem_drv.h
[modify] https://crrev.com/abf3ddd5738f325063ed129208813db819aa2e5e/drivers/gpu/drm/vgem/vgem_dma_buf.c

Labels: -Hotlist-Merge-Approved Merge-Merged
Status: Fixed (was: Started)
Project Member

Comment 8 by sheriffbot@chromium.org, Feb 12 2018

Cc: diand...@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-65

Sign in to add a comment