Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
> > reg_read() callback registered with nvmem core expects an integer error
> > as a return value but rmem_read() returns the number of bytes read, as a
> > result error checks in nvmem core fail even when they shouldn't.
> >
> > Return 0 on success where number of bytes read match the number of bytes
> > requested and a negative error -EINVAL on all other cases.
> >
> > Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Joy Chakraborty <joychakr@xxxxxxxxxx>
> > ---
> >  drivers/nvmem/rmem.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> > index 752d0bf4445e..a74dfa279ff4 100644
> > --- a/drivers/nvmem/rmem.c
> > +++ b/drivers/nvmem/rmem.c
> > @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
> >
> >       memunmap(addr);
> >
> > -     return count;
> > +     if (count != bytes) {
> > +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> > +             return -EINVAL;
>
> Why is a "short read" somehow illegal here?  What internal changes need
> to be made now that this has changed?

In my opinion "short read" should be illegal for cases where if the
nvmem core is unable to read the required size of data to fill up a
nvmem cell then data returned might have truncated value.

No internal changes should be made since the registered reg_read() is
called from  __nvmem_reg_read() which eventually passes on the error
code to nvmem_reg_read() whose return code is already checked and
passed to nvmem consumers.
Currently rmem driver is incorrectly passing a positive value for success.

>
> And what will userspace do with this error message in the kernel log?

User space currently is not seeing this error for nvmem device/eeprom
reads due to the following code at nvmem/core.c in
bin_attr_nvmem_read():
"
    rc = nvmem_reg_read(nvmem, pos, buf, count);

    if (rc)
        return rc;

    return count;
"
since it expects to return the number of bytes.

Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read() in current code, which should be fixed on
returning 0 for success.

> thanks,
>
> greg k-h

Thanks
Joy