nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code)

[Andy Ritger]

On Tue, Apr 11, 2017 at 09:24:33AM +0200, Lukas Wunner wrote:
> Hi Nikhil,
> 
> On Tue, Apr 11, 2017 at 09:44:35AM +0530, Nikhil Mahale wrote:
> > There is not interest in relaxing the export of refcount_inc, and 
> > changing the license of nvidia-drm.ko isn't viable right now.
> 
> Why not dual-license as MIT+GPL?

We intentionally chose MODULE_LICENSE("MIT") for nvidia-drm.ko, rather
than MODULE_LICENSE("Dual MIT/GPL"), to avoid any ambiguity:

* nvidia-drm.ko depends on nvidia.ko, which is MODULE_LICENSE("NVIDIA").

* nvidia-drm.ko is the portion of the NVIDIA dGPU driver that interfaces
  with DRM in the kernel.  We wouldn't want nvidia-drm.ko to
  inadvertently function as, or even be perceived as, a path for
  nvidia.ko to indirectly get access to EXPORT_SYMBOL_GPL symbols.

> > * Make these static inline DRM functions EXPORT_SYMBOL instead of 
> > inline.
> 
> Intuitively, I'd say wrapping a function declared EXPORT_SYMBOL_GPL
> in another function declared EXPORT_SYMBOL doesn't seem like a legal
> way to solve this issue.

I understand why people may have that concern.

But to me, it seems like there is a difference between
refcount_dec_and_test() and drm_crtc_commit_put().  The former is a
kernel-internal implementation detail, where EXPORT_SYMBOL_GPL() seems
appropriate.  The latter is an interface exposed by DRM, with the intent
to enable DRM drivers to use it.  That drm_crtc_commit_put() is one-line
long and simply calls kref_put()/refcount_dec_and_test() seems like an
implementation detail of drm_crtc_commit_put(), and independent of the
interface DRM provides to its drivers.

Maybe there are better options, but of the options Nikhil outlined:

> * Make these static inline DRM functions EXPORT_SYMBOL instead of inline.
>
> * Make these static inline DRM functions not use kref.
>
> * Make nvidia-drm.ko not use these static inline DRM functions.

The first one seems sort of reasonable to me, given the above reasoning.
It also seems somewhat in the spirit of allowing DRM to be usable on a
variety of differently-licensed platforms.

To be fair, it would be unfortunate to lose the static inlineness of
those functions.

> > My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers 
> > team.
> > 
> > I have been working on adding DRM KMS support to our driver. The NVIDIA 
> > GPU driver package (364.12 and higher) provides a kernel module, 
> > nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM 
> > driver with the DRM subsystem of the Linux kernel and advertises KMS 
> > capability on Linux kernel v4.1 or higher, with CONFIG_DRM and 
> > CONFIG_DRM_KMS_HELPER enabled.
> 
> Sorry to hijack this thread, but there's an egregious, long-standing bug
> in your driver with regards to vgaarb usage:  nvidia/nv.c calls
> vga_tryget() but never calls vga_put(), in other words your driver locks
> legacy VGA I/O but never unlocks it.  This is in the proprietary, non-MIT
> licensed portion of your driver.
> 
> The bug was already reported to Nvidia four years ago:
> https://devtalk.nvidia.com/default/topic/545560
> 
> It causes issues such as deadlocks or inability to control backlight
> on MacBook Pros, commit 4eebd5a4e726 tried to work around it but
> introduced more problems.
> 
> Could you please look into fixing that bug? Thanks!

Thanks for pointing that out.  Yes, we'll take a look.

Thanks,
- Andy


> Lukas
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel