Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 25 May 2020 22:19:02 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [RFC PATCH] gpio: uapi: v2 proposal
Message-ID: <20200525141902.GA27938@sol>
References: <20200516064507.19058-1-warthog618@gmail.com>
 <CACRpkdbputuoHFWL_FhUNR-ZywvJt=qYdaa+i2cLt-Odmgxe2w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACRpkdbputuoHFWL_FhUNR-ZywvJt=qYdaa+i2cLt-Odmgxe2w@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 25, 2020 at 10:39:42AM +0200, Linus Walleij wrote:
> On Sat, May 16, 2020 at 8:45 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> > Add a new version of the uAPI to address existing 32/64bit alignment
> > issues, add support for debounce, and provide some future proofing by
> > adding padding reserved for future use.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> 
> I don't see any major problems with it, just some comments:
> 
> + * @padding: reserved for future use and should be zero filled
> 
> *MUST* be zerofilled is what it should say.
> 
> > +struct gpioline_config {
> > +       __u8 default_values[GPIOLINES_MAX];
> 
> So 32 bytes
> 

Actually that one is 64 bytes, which is the same as v1, i.e. GPIOLINES_MAX
is the same as GPIOHANDLES_MAX - just renamed.

On the subject of values, is there any reason to use a byte for each line
rather value than a bit?

> > +       __u32 flags;
> > +       __u8 direction;
> > +       __u8 drive;
> > +       __u8 bias;
> > +       __u8 edge_detection;
> 
> Some comment in the struc that this adds up to 32 bits?
> 
> > +       __u32 debounce;
> > +       __u32 padding[7]; /* for future use */
> 
> What we need to do inside the kernel with all of these
> is to ascertain that they are 0 for now when they are
> sent in and refuse them otherwise, I think it was Lars-Peter
> Clausen who said that they had to retire a whole slew of
> ioctl()s because some userspace just used unallocated
> memory for this and since that was random bytes it needed to
> be supported with that content forever and the reserved
> padding could never be used for the reserved purpose.
> 
> This kind of comment goes for all the structs.
> 

OK, I wasn't sure how strict the validation should be on the kernel
side, but I'll take a strict stance and check the whole struct.

Having said that, when adding future fields, the idea was to have a bit
in the flags that indicates that the corresponding field is now valid.
If the flag is not set then whatever value is there is irrelevant.
e.g. the debounce field value is irrelevent and ignored unless the
GPIOLINE_FLAG_V2_DEBOUNCE is set in flags.
OTOH, if the bit is set then the field would have to be set correctly.
And the current kernel would reject a future request with an unsupported
bit set in flags.

But definitely better to play it safe - will check the padding is
zeroed as well, as well as any field for which the flag bit is clear.

> But mostly it is a question about the kernel code receiving
> or emitting these.
> 

For sure - all the structs returned will be zeroed before use so as not
to leak kernel stack - unless they originate from userspace of course.

Back on retired ioctls, I notice that 5, 6, and 7 are missing from gpio.
Have those been retired, or just skipped over by accident?

Cheers,
Kent.