linux-kernel - Re: [External] Re: [PATCH v3 2/2] misc: pvpanic: introduce module parameter 'events'

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: <a85467db-f6d8-4ac3-1be4-af31c881616f@bytedance.com>
Date:   Sun, 10 Jan 2021 11:10:43 +0800
From:   zhenwei pi <pizhenwei@...edance.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     arnd@...db.de, linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH v3 2/2] misc: pvpanic: introduce module
 parameter 'events'

On 1/9/21 7:31 PM, Greg KH wrote:
> On Fri, Jan 08, 2021 at 04:26:17PM +0100, Paolo Bonzini wrote:
>> On 08/01/21 16:15, Greg KH wrote:
>>> On Fri, Jan 08, 2021 at 04:04:24PM +0100, Paolo Bonzini wrote:
>>>> On 08/01/21 15:07, Greg KH wrote:
>>>>>>     static void __iomem *base;
>>>>>> +static unsigned int events = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
>>>>>> +module_param(events, uint, 0644);
>>>>>> +MODULE_PARM_DESC(events, "set event limitation of pvpanic device");
>>>>> I do not understand you wanting a module parameter as well as a sysfs
>>>>> file.  Why is this needed?  Why are you spreading this information out
>>>>> across different apis and locations?
>>>>
>>>> It can be useful to disable some functionality, for example in case you want
>>>> to fake running on an older virtualization host.  This can be done for
>>>> debugging reasons, or to keep uniform handling across a fleet that is
>>>> running different versions of QEMU.
>>>
>>> And where is this all going to be documented?
>>
>> I don't disagree.
>>
>>> And what's wrong with just making the sysfs attribute writable?
>>
>> Isn't it harder to configure it at boot?  Also the sysfs attribute added by
>> patch 1 is documenting what is supported by the device, while the module
>> parameter can be set to any value (you can think of the module parameter as
>> of a "what to log" option, except the logging happens on another machine).
> 
> But the module parameter is global, and not device specific.
> 
> And yes, it would be harder to configure this at boot, is this something
> that is required?  If so, please document that somewhere.
> 
>> Therefore, if you make the sysfs attribute writable, you would actually need
>> _two_ attributes, one for the in-use capabilities and one for the device
>> capabilities.  And sysfs files are runtime values, which is different
>> concept than 0444 module parameters (which are more like just
>> configuration).
> 
> That's not the module parameter mode setting in this patch :(
> 
>> So you would have to decide whether it's valid to write 2
>> to the in-use capabilities file when the device capabilities are "1", and I
>> don't really have a good answer for that.
>>
>> Also considering that there will not be more than one copy of this device
>> (it doesn't make sense as they would all do exactly the same thing), in this
>> case a module parameter really seems to be the simplest way to configure it.
> 
> So you never can have more than one of these in the system at one time?
> Because if this ever becomes not true, the module parameter is a mess...
> 
> thanks,
> 
> greg k-h
> 

What about adding _two_ device attribute:
capability (0444): detect from device which the hypervisor really supports.

events (0644): root user could enable/disable feature(s) from guest side.

-- 
zhenwei pi