linux-kernel - Re: [PATCH 14/14] [media] fix warning on v4l2_subdev

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:   Mon, 17 Jul 2017 16:32:18 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Tejun Heo <tj@...nel.org>, Guenter Roeck <linux@...ck-us.net>,
        IDE-ML <linux-ide@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Niklas Söderlund <niklas.soderlund@...natech.se>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        Daeseok Youn <daeseok.youn@...il.com>,
        Alan Cox <alan@...ux.intel.com>,
        adi-buildroot-devel@...ts.sourceforge.net,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        devel@...verdev.osuosl.org
Subject: Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result
 interpreted as bool

On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@...all.nl> wrote:
>> On 14/07/17 11:36, Arnd Bergmann wrote:
>>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>>>        * fmt->fmt.sliced under valid calling conditions
>>>        */
>>> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>>> -             return -EINVAL;
>>> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>>> +     if (ret)
>>> +             return ret;
>>
>> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
>> break something.
> 
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
> 
> I guess in either case, looking at the callers more carefully would be
> a good idea.

The subtle problem here is that v4l2_subdev_call will return -ENOIOCTLCMD if
ops->vbi->g_sliced_fmt == NULL, which typically is not returned to userspace
but either ignored or replaced by another error. It indicates that the
sub device doesn't implement this operation, and it depends on the context
and the operation whether or not that is to be considered an error.

I have no clue what is expected here, without digging deep in the code.

Better to keep it as-is. It really isn't important to waste time on this.

> 
>>> -     return 0;
>>> +     return ret;
>>>  }
>>>
>>>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>>
>>
>> This is all very hackish, though. I'm not terribly keen on this patch. It's not
>> clear to me *why* these warnings appear in your setup.
> 
> it's possible that this only happened with 'ccache', which first preprocesses
> the source and the passes it with v4l2_subdev_call expanded into the
> compiler. This means the line looks like
> 
>         if ((!(cx->sd_av) ? -ENODEV :
>             (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
>                (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
> &fmt->fmt.sliced) :
>                -ENOIOCTLCMD))
> 
> The compiler now complains about the sub-expression that it sees for
> cx->sd_av==NULL:
> 
>    if (-ENODEV)
> 
> which it considers nonsense because it is always true and the value gets
> ignored.
> 
> Let me try again without ccache for now and see what warnings remain.
> We can find a solution for those first, and then decide how to deal with
> ccache.

Sounds good.

I'm OK with applying this if there is no other way to prevent these warnings.

Regards,

	Hans

> 
>         Arnd
>