Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver

[Catangiu, Adrian Costin]

Hi Greg,

After your previous reply on this thread we started considering to provide this 
interface and framework/functionality through a userspace service instead of a 
kernel interface.
The latest iteration on this evolving patch-set doesn’t have strong reasons for 
living in the kernel anymore - the only objectively strong advantage would be 
easier driving of ecosystem integration; but I am not sure that's a good enough 
reason to create a new kernel interface.

I am now looking into adding this through Systemd. Either as a pluggable 
service or maybe even a systemd builtin offering.

What are your thoughts on it?

Thanks,
Adrian.

On 23/03/2021, 14:57, "Greg KH" <gregkh@linuxfoundation.org> wrote:

    CAUTION: This email originated from outside of the organization. Do not 
click links or open attachments unless you can confirm the sender and know the 
content is safe.



    On Mon, Mar 08, 2021 at 05:03:58PM +0100, Alexander Graf wrote:
    >
    >
    > On 08.03.21 15:36, Greg KH wrote:
    > >
    > > On Mon, Mar 08, 2021 at 04:18:03PM +0200, Adrian Catangiu wrote:
    > > > +static struct miscdevice sysgenid_misc = {
    > > > +     .minor = MISC_DYNAMIC_MINOR,
    > > > +     .name = "sysgenid",
    > > > +     .fops = &fops,
    > > > +};
    > >
    > > Much cleaner, but:
    > >
    > > > +static int __init sysgenid_init(void)
    > > > +{
    > > > +     int ret;
    > > > +
    > > > +     sysgenid_data.map_buf = get_zeroed_page(GFP_KERNEL);
    > > > +     if (!sysgenid_data.map_buf)
    > > > +             return -ENOMEM;
    > > > +
    > > > +     atomic_set(&sysgenid_data.generation_counter, 0);
    > > > +     atomic_set(&sysgenid_data.outdated_watchers, 0);
    > > > +     init_waitqueue_head(&sysgenid_data.read_waitq);
    > > > +     init_waitqueue_head(&sysgenid_data.outdated_waitq);
    > > > +     spin_lock_init(&sysgenid_data.lock);
    > > > +
    > > > +     ret = misc_register(&sysgenid_misc);
    > > > +     if (ret < 0) {
    > > > +             pr_err("misc_register() failed for sysgenid\n");
    > > > +             goto err;
    > > > +     }
    > > > +
    > > > +     return 0;
    > > > +
    > > > +err:
    > > > +     free_pages(sysgenid_data.map_buf, 0);
    > > > +     sysgenid_data.map_buf = 0;
    > > > +
    > > > +     return ret;
    > > > +}
    > > > +
    > > > +static void __exit sysgenid_exit(void)
    > > > +{
    > > > +     misc_deregister(&sysgenid_misc);
    > > > +     free_pages(sysgenid_data.map_buf, 0);
    > > > +     sysgenid_data.map_buf = 0;
    > > > +}
    > > > +
    > > > +module_init(sysgenid_init);
    > > > +module_exit(sysgenid_exit);
    > >
    > > So you do this for any bit of hardware that happens to be out there?
    > > Will that really work?  You do not have any hwid to trigger off of to
    > > know that this is a valid device you can handle?
    >
    > The interface is already useful in a pure container context where the
    > generation change request is triggered by software.
    >
    > And yes, there are hardware triggers, but Michael was quite unhappy about
    > potential races between VMGenID change and SysGenID change and thus wanted
    > to ideally separate the interfaces. So we went ahead and isolated the
    > SysGenID one, as it's already useful as is.
    >
    > Hardware drivers to inject change events into SysGenID can then follow
    > later, for all different hardware platforms. But SysGenID as in this patch
    > is a completely hardware agnostic concept.

    Ok, this is going to play havoc with fuzzers and other "automated
    testers", should be fun to watch!  :)

    Let's queue this up and see what happens...

    thanks,

    greg k-h




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar 
Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in 
Romania. Registration number J22/2621/2005.