linux-kernel - Re: [PATCH] VFIO: platform: AMD xgbe reset module

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7287273.XGFpHqQ9fN@wuerfel>
Date:	Thu, 15 Oct 2015 13:21:55 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Eric Auger <eric.auger@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, eric.auger@...com,
	alex.williamson@...hat.com, b.reynal@...tualopensystems.com,
	kvmarm@...ts.cs.columbia.edu, kvm@...r.kernel.org,
	thomas.lendacky@....com, patches@...aro.org,
	christoffer.dall@...aro.org, suravee.suthikulpanit@....com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] VFIO: platform: AMD xgbe reset module

On Thursday 15 October 2015 10:08:02 Eric Auger wrote:
> Hi Arnd,
> On 10/14/2015 05:38 PM, Arnd Bergmann wrote:
> > On Wednesday 14 October 2015 15:33:12 Eric Auger wrote:
> >> --- a/drivers/vfio/platform/vfio_platform_common.c
> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
> >> @@ -31,6 +31,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> >>                 .reset_function_name = "vfio_platform_calxedaxgmac_reset",
> >>                 .module_name = "vfio-platform-calxedaxgmac",
> >>         },
> >> +       {
> >> +               .compat = "amd,xgbe-seattle-v1a",
> >> +               .reset_function_name = "vfio_platform_amdxgbe_reset",
> >> +               .module_name = "vfio-platform-amdxgbe",
> >> +       },
> >>  };
> >>  
> >>  static void vfio_platform_get_reset(struct vfio_platform_device *vdev,
> >>
> > 
> > This is causing build errors for me when CONFIG_MODULES is disabled.
> Sorry about that and thanks for reporting the issue
> > 
> > Could this please be restructured so vfio_platform_get_reset does
> > not attempt to call __symbol_get() but instead has the drivers
> > register themselves properly to a subsystem?
> OK
> 
> Could you elaborate about "has the drivers register themselves properly
> to a subsystem".
> 
> My first proposal when coping with this problematic of being able to add
> reset plugins to the vfio-platform driver was to create new drivers per
> device requiring reset. but this was considered painful for end-users,
> who needed to be aware of the right driver to bind - and I think that
> makes sense - (https://lkml.org/lkml/2015/4/17/568) .

Having multiple drivers indeed sucks, but your current approach isn't
that much better, as you still have two modules that are used to driver
the same hardware.

I would expect that the same driver that is used for the normal
operation and that it calls a function to register itself to vfio
by passing a structure with the device and reset function pointer.

> A naive question I dare to ask, wouldn't it be acceptable to make
> vfio_platform depend on CONFIG_MODULES? Don't we disable modules for
> security purpose? In that context would we use VFIO?

I think a lot of embedded systems turn off modules to save a little
memory, speed up boot time and simplify their user space.

Aside from that, the current method is highly unusual and looks a bit
fragile to me, as you are relying on internals of the module loader
code. It's also a layering violation as the generic code needs to be
patched for each device specific module that is added, and we try
to avoid that.

A possible solution could be something inside the xgbe driver like


static void xgbe_init_module(void)
{
	int ret = 0;

	if (IS_ENABLED(CONFIG_AMD_XGBE_ETHERNET)
		ret = platform_driver_register(&xgbe_driver);
	if (ret)
		return ret;

	if (IS_ENABLED(CONFIG_VFIO_PLATFORM))
		ret = vfio_platform_register_reset(&xgbe_of_match, xgbe_platform_reset);

	return ret;	
}

This way you have exactly one driver module that gets loaded for the
device and you can use it either with the platform_driver or through
vfio.

A nicer way that would be a little more work would be to integrate
the reset infrastructure into 'struct platform_driver' framework,
by adding another callback to the it for doing the interaction with
vfio, something like

enum vfio_platform_op {
	VFIO_PLATFORM_BIND,
	VFIO_PLATFORM_UNBIND,
	VFIO_PLATFORM_RESET,
};

struct platform_driver {
        int (*probe)(struct platform_device *);
        int (*remove)(struct platform_device *);
	...
	int (*vfio_manage)(struct platform_device *, enum vfio_platform_op);
        struct device_driver driver;
};

This would integrate much more closely into the platform driver framework,
just like the regular vfio driver integrates into the PCI framework.
Unlike PCI however, you can't just use the generic driver framework to
unbind the driver, because you still need device specific code.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/