linux-kernel - Re: [PATCH v3 1/7] seqnum_ops: Introduce Sequence Number Ops

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: <20210205221621.GA74059@agluck-desk2.amr.corp.intel.com>
Date:   Fri, 5 Feb 2021 14:16:21 -0800
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>, corbet@....net,
        peterz@...radead.org, keescook@...omium.org, rafael@...nel.org,
        lenb@...nel.org, james.morse@....com, bp@...en8.de,
        devel@...verdev.osuosl.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v3 1/7] seqnum_ops: Introduce Sequence Number Ops

On Fri, Feb 05, 2021 at 01:03:18PM -0700, Shuah Khan wrote:
> On 2/5/21 2:58 AM, Greg KH wrote:
> > On Wed, Feb 03, 2021 at 11:11:57AM -0700, Shuah Khan wrote:
> > > +static inline u32 seqnum32_inc(struct seqnum32 *seq)
> > > +{
> > > +	atomic_t val = ATOMIC_INIT(seq->seqnum);
> > > +
> > > +	seq->seqnum = (u32) atomic_inc_return(&val);
> > > +	if (seq->seqnum >= UINT_MAX)
> > > +		pr_info("Sequence Number overflow %u detected\n",
> > > +			seq->seqnum);
> > > +	return seq->seqnum;
> > 
> > As Peter points out, this is doing doing what you think it is doing :(
> > 
> > Why do you not just have seq->seqnum be a real atomic variable?  Trying
> > to switch to/from one like this does not work as there is no
> > "atomic-ness" happening here at all.
> > 
> 
> Yes. This is sloppy on my part. As Peter and Rafael also pointed. I have
> to start paying more attention to my inner voice.

What's the end goal with this sequence number type?

Apart from the functional issues with this code already pointed
out, it doesn't seem overly useful to just print the *value* of
the sequence number when it hits the max value.  It might be
more helpful if each instance had a seq->name field that is
used here to tell the user *which* user of sequence numbers
had seen the wrap arounnd.

But that just begs the question of what should users of sequence
numbers do when wrap around occurs? Any user that can run through
sequence numbers at greater than 10 Hz is going to cause a problem
for systems that stay running for years.

Maybe you should force users to code for wraparound by initializing
to something like 0xffffff00 so that they all get a wraparound event
quickly, rather than slowly, (same as was done with jiffies)?