| 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
| ||
|
Message-ID: <DM6PR11MB46577B2C09A1D3A8EFAAD4039B749@DM6PR11MB4657.namprd11.prod.outlook.com> Date: Thu, 11 May 2023 20:53:19 +0000 From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com> To: Jiri Pirko <jiri@...nulli.us> CC: Vadim Fedorenko <vadfed@...a.com>, Jakub Kicinski <kuba@...nel.org>, Jonathan Lemon <jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>, "Olech, Milena" <milena.olech@...el.com>, "Michalik, Michal" <michal.michalik@...el.com>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Vadim Fedorenko <vadim.fedorenko@...ux.dev> Subject: RE: [RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML >From: Jiri Pirko <jiri@...nulli.us> >Sent: Thursday, May 11, 2023 10:14 AM > >Thu, May 11, 2023 at 09:38:04AM CEST, arkadiusz.kubalewski@...el.com wrote: [...] >>>>+ - >>>>+ name: holdover >>>>+ doc: | >>>>+ dpll is in holdover state - lost a valid lock or was forced by >>>>+ selecting DPLL_MODE_HOLDOVER mode >>> >>>Is it needed to mention the holdover mode. It's slightly confusing, >>>because user might understand that the lock-status is always "holdover" >>>in case of "holdover" mode. But it could be "unlocked", can't it? >>>Perhaps I don't understand the flows there correctly :/ >>> >> >>Yes, it could be unlocked even when user requests the 'holdover' mode, >i.e. >>when the dpll was not locked to a valid source before requesting the mode. >>Improved the docs: >> name: holdover >> doc: | >> dpll is in holdover state - lost a valid lock or was forced >> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >> if it was not, the dpll's lock-status will remain >> DPLL_LOCK_STATUS_UNLOCKED even if user requests >> DPLL_MODE_HOLDOVER) >>Is that better? > >See my comment to this in the other branch of this thread. > Sure, gonna reply there. [...] >>>>+ - >>>>+ name: int-oscillator >>>>+ doc: device internal oscillator >>> >>>Is this somehow related to the mode "nco" (Numerically Controlled >>>Oscillator)? >>> >> >>Yes. > >How? Why do we need to expose it as a pin then? > Sorry, I messed up something with that answer.. It is not related to NCO (as NCO uses SYSTEM CLOCK to produce frequency). It is a type of a pin which source or output is somehow internal. I.e. our dpll's can syntonize to the 1 PPS clock signal produced by network chip. As for other use-cases it could serve as way of having one or more oscillators on board connected to input pins. [...] >>>>+ type: enum >>>>+ name: event >>>>+ doc: events of dpll generic netlink family >>>>+ entries: >>>>+ - >>>>+ name: unspec >>>>+ doc: invalid event type >>>>+ - >>>>+ name: device-create >>>>+ doc: dpll device created >>>>+ - >>>>+ name: device-delete >>>>+ doc: dpll device deleted >>>>+ - >>>>+ name: device-change >>> >>>Please have a separate create/delete/change values for pins. >>> >> >>Makes sense, but details, pin creation doesn't occur from uAPI perspective, >>as the pins itself are not visible to the user. They are visible after they >>are registered with a device, thus we would have to do something like: >>- pin-register >>- pin-unregister >>- pin-change >> >>Does it make sense? > >>From perspective of user, it is "creation/new" or "deletion/del". >Object appears of dissapears in UAPI, no matter how this is implemented >in kernel. If you call it register/unregister, that exposes unnecessary >internal kernel notation. > >No strong feeling though if you insist on register/unregister, it just >sounds odd and funny. > >Anyway, one way or another, be in-sync naming wise with device events. > Sure going in sync with device event names seems best option, will fix as suggested: - pin-create - pin-delete - pin-change > > >> >>> >>>>+ doc: | >>>>+ attribute of dpll device or pin changed, reason is to be >found >>>>with >>>>+ an attribute type (DPLL_A_*) received with the event >>>>+ >>>>+ >>>>+attribute-sets: >>>>+ - >>>>+ name: dpll >>>>+ enum-name: dplla >>>>+ attributes: >>>>+ - >>>>+ name: device >>>>+ type: nest >>>>+ value: 1 >>> >>>Why not 0? >>> >> >>Sorry I don't recall what exact technical reasons are behind it, but all >>netlink attributes I have found have 0 value attribute unused/unspec. > >I don't see why that is needed, I may be missing something though. >Up to you. > Will leave it as is, if no other comments. [...] >>>>+ attribute-set: dpll >>>>+ flags: [ admin-perm ] >>> >>>I may be missing something, but why do you enforce adming perm for >>>get/dump cmds? >>> >> >>Yes, security reasons, we don't want regular users to spam-query the driver >>ops. Also explained in docs: >>All netlink commands require ``GENL_ADMIN_PERM``. This is to prevent >>any spamming/D.o.S. from unauthorized userspace applications. > >Hmm, I wonder why other read cmds usually don't need this. In fact, >is there some read netlink cmd in kernel now which needs it? > Seems drivers/net/team/team.c uses it for get, but anyway this is a "least privilege" security principle, if regular user cannot do anything with that information, there is no point on providing it. > >> >>> >>>>+ >>>>+ do: >>>>+ pre: dpll-pre-doit >>>>+ post: dpll-post-doit >>>>+ request: >>>>+ attributes: >>>>+ - id >>>>+ - bus-name >>>>+ - dev-name >>>>+ reply: >>>>+ attributes: >>>>+ - device >>>>+ >>>>+ dump: >>>>+ pre: dpll-pre-dumpit >>>>+ post: dpll-post-dumpit >>>>+ reply: >>>>+ attributes: >>>>+ - device >>> >>>I might be missing something, but this means "device" netdev attribute >>>DPLL_A_DEVICE, right? If yes, that is incorrect and you should list all >>>the device attrs. >>> >> >>Actually this means that attributes expected in response to this command are >>from `device` subset. >>But I see your point, will make `device` subset only for pin's nested >>attributes, and here will list device attributes. > >Yes, that is my point. The fix you describes sounds fine. > > Thank you! Arkadiusz [...]
Powered by blists - more mailing lists