netdev - Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

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: Wed, 26 Jun 2024 19:42:47 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Danielle Ratson <danieller@...dia.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"corbet@....net" <corbet@....net>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>,
	"sdf@...gle.com" <sdf@...gle.com>,
	"kory.maincent@...tlin.com" <kory.maincent@...tlin.com>,
	"maxime.chevallier@...tlin.com" <maxime.chevallier@...tlin.com>,
	"vladimir.oltean@....com" <vladimir.oltean@....com>,
	"przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
	"ahmed.zaki@...el.com" <ahmed.zaki@...el.com>,
	"richardcochran@...il.com" <richardcochran@...il.com>,
	"shayagr@...zon.com" <shayagr@...zon.com>,
	"paul.greenwalt@...el.com" <paul.greenwalt@...el.com>,
	"jiri@...nulli.us" <jiri@...nulli.us>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	mlxsw <mlxsw@...dia.com>, Ido Schimmel <idosch@...dia.com>,
	Petr Machata <petrm@...dia.com>
Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
 supporting CDB commands

> > Please could you test it.
> > 
> > 65535 jiffies is i think 655 seconds? That is probably too long to loop when
> > the module has been ejected. Maybe replace it with HZ?
> > 
> 
> Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.

I _think_ it depends on CONFIG_HZ, which can be 100, 250, 300 and
1000.

> The test you are asking for is a bit complicated since I don’t have
> a machine physically nearby, do you find it very much important?

> I mean, it is not very reasonable thing to do, burning fw on a
> module and in the exact same time eject it.

Shooting yourself in the foot is not a very reasonable thing to do,
but the Unix philosophy is to all root to do it. Do we really want 60
to 600 seconds of the kernel spamming the log when somebody does do
this?

> > Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> > delay before the continue.
> > 
> > > > > > +		}
> > > > > > +
> > > > > > +		if ((*cond_success)(rpl.state))
> > > > > > +			return 0;
> > > > > > +
> > > > > > +		if (*cond_fail && (*cond_fail)(rpl.state))
> > > > > > +			break;
> > > > > > +
> > > > > > +		msleep(20);
> > > > > > +	} while (time_before(jiffies, end));
> > > > >

> > O.K. Please evaluate the condition again after the while() just so ETIMEDOUT is
> > not returned in error.
> 
> Not sure I understood.
> Do you want to have one more polling in the end of the loop? What could return ETIMEDOUT?

Consider what happens when msleep(20) actually sleeps a lot longer.

Look at the core code which gets this correct:

#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
                                sleep_before_read, args...) \
({ \
        u64 __timeout_us = (timeout_us); \
        unsigned long __sleep_us = (sleep_us); \
        ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
        might_sleep_if((__sleep_us) != 0); \
        if (sleep_before_read && __sleep_us) \
                usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
        for (;;) { \
                (val) = op(args); \
                if (cond) \
                        break; \
                if (__timeout_us && \
                    ktime_compare(ktime_get(), __timeout) > 0) { \
                        (val) = op(args); \
                        break; \
                } \
                if (__sleep_us) \
                        usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
                cpu_relax(); \
        } \
        (cond) ? 0 : -ETIMEDOUT; \
})

So after breaking out of the for loop with a timeout, it evaluates the
condition one more time, and uses that to decide on 0 or ETIMEDOUT. So
it does not matter if usleep_range() range slept for 60 seconds, not
60ms, the exit code will be correct.

      Andrew