Re: [PATCH net-next] net: mdio: Add netlink interface

[Sean Anderson]

On 3/7/23 12:23, Andrew Lunn wrote:
>> Yes, and I should probably have commented on this in the commit message.
>> IMO the things you listed are... iffy but unlikely to cause a
>> malfunction.
> 
> You consider a missed interrupt not a malfunction?

Hm, yeah that would probably do it.

>> >> +
>> >> +	for (insn = xfer->prog, pc = 0;
>> >> +	     pc < xfer->prog_len;
>> >> +	     insn = &xfer->prog[++pc]) {
>> >> +		if (time_after(jiffies, timeout)) {
>> >> +			ret = -ETIMEDOUT;
>> >> +			break;
>> >> +		}
>> >> +
>> >> +		switch ((enum mdio_nl_op)insn->op) {
>> >> +		case MDIO_NL_OP_READ:
>> >> +			phy_id = __arg_ri(insn->arg0, regs);
>> >> +			prtad = mdio_phy_id_prtad(phy_id);
>> >> +			devad = mdio_phy_id_devad(phy_id);
>> >> +			reg = __arg_ri(insn->arg1, regs);
>> >> +
>> >> +			if (mdio_phy_id_is_c45(phy_id))
>> >> +				ret = __mdiobus_c45_read(xfer->mdio, prtad,
>> >> +							 devad, reg);
>> >> +			else
>> >> +				ret = __mdiobus_read(xfer->mdio, phy_id, reg);
>> > 
>> > The application should say if it want to do C22 or C45.
>> 
>> The phy_id comes from the application. So it sets MDIO_PHY_ID_C45 if it wants
>> to use C45.
> 
> Ah, i misunderstood what mdio_phy_id_is_c45() does.
> 
> Anyway, i don't like MDIO_PHY_ID_C45, it has been pretty much removed
> everywhere with the refactoring of MDIO drivers to export read and
> read_c45 etc. PHY drivers also don't use it, they use c22 or c45
> specific methods. So i would prefer an additional attribute. That also
> opens up the possibility of adding C45 over C22.

Well, this is really just because there is an existing way to specify c22
and c45 addresses in a u16. We could definitely add a "please do C45 over
C22" flag. That said, I think that sort of thing is handled better by
allowing writes in the general case.

>> As Russell noted, this is dangerous in the general case.
> 
> And Russell also agreed this whole module is dangerous in general.
> Once you accept it is dangerous, its a debug tool only, why not allow
> playing with a bit more fire? You could at least poke around the MDIO
> bus structures and see if a PHY has been found, and it not, block C45
> over C22.

I can look into that.

>> >> +			if (mdio_phy_id_is_c45(phy_id))
>> >> +				ret = __mdiobus_c45_write(xfer->mdio, prtad,
>> >> +							  devad, reg, val
>> >> +			else
>> >> +				ret = __mdiobus_write(xfer->mdio, dev, reg,
>> >> +						      val);
>> >> +#else
>> >> +			ret = -EPERM;
>> > 
>> > EPERM is odd, EOPNOTSUPP would be better. EPERM suggests you can run
>> > it as root and it should work.
>> 
>> Well, EPERM is what you get when trying to write a 444 file, which is
>> effectively what we're enforcing here.
> 
> Does it change to 644 when write is enabled?

Yes. But it is more like 400 and 600.

> But netlink does not even use file access permissions.

Is EPERM reserved only for files?

> I would probably trap this earlier, where you have a extack instance
> you can return a meaningful error message string.

That sounds good.

--Sean