netdev - Re: [PATCH 1/3] net: phylib: add adjust

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]

Date:	Thu, 5 Jun 2014 11:12:09 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Daniel Mack <daniel@...que.org>
Cc:	netdev <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	"marek.belisko" <marek.belisko@...il.com>,
	Matus Ujhelyi <ujhelyi.m@...il.com>
Subject: Re: [PATCH 1/3] net: phylib: add adjust_state callback to phy device

Hi Daniel,

2014-06-05 0:14 GMT-07:00 Daniel Mack <daniel@...que.org>:
> Hi Florian,
>
> On 06/05/2014 07:11 AM, Florian Fainelli wrote:
>> 2014-06-04 2:00 GMT-07:00 Daniel Mack <zonque@...il.com>:
>>> Allow phy drivers to take action when the core does its link adjustment.
>>> No change for drivers that do not implement this callback.
>>
>> This sounds potentially dangerous if misused, PHY drivers would
>> basically be allowed to do arbitrary link state changes based on their
>> custom needs.
>
> Yes, and this is basically what my quirk handler does. It takes action
> when the link goes down (PHY_NOLINK), as we unfortunately need to reset
> the chip every time this happens.

In fact, the callback name is sort of a misnomer here, as you are not
really adjusting the PHY device state here, you are reading from it to
do something about the PHY device based on this state.

Could you rename it to "state_notify" for instance to make it clear
that this must absolutely be a read-only operation and the PHY driver
is by no means allow to mess with the PHY device structure at all?
Constifying the phy_device argument here would not really help
unfortunately, and providing you with just the 'state' as a RO
argument would not help either since you need to access the PHY
device...

>
>> I really need to review your workaround here to better
>> understand whether we can come up with a solution that allows for less
>> liberty in PHY drivers.
>
> Sure, any other way of calling back to the PHY when the core enters the
> PHY_NOLINK state would do. I just thought that an adjust_state callback
> is most versatile, but I understand that it could be misused.

I was thinking about having a notifier callchain here that would
execute synchronously and in the same context as your proposed
adjust_link() callback, although that might be be too heavy weighted
for basically just one notified callback.

>
>
> Thanks for having a look!
>
> Daniel
>

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html