From: Ezequiel Garcia <ezequiel@collabora.com>
To: Shaobo He <shaobo@cs.utah.edu>, linux-media@vger.kernel.org
Cc: Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
Heiko Stuebner <heiko@sntech.de>,
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
Kamil Debski <kamil@wypas.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Andrzej Hajda <a.hajda@samsung.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Rick Chang <rick.chang@mediatek.com>,
"open list:ARM/Rockchip SoC support"
<linux-rockchip@lists.infradead.org>,
Hans Verkuil <hans.verkuil@cisco.com>,
Mikhail Ulyanov <mikhail.ulyanov@cogentembedded.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Bin Liu <bin.liu@mediatek.com>,
Anton Leontiev <scileont@gmail.com>,
Kees Cook <keescook@chromium.org>, Benoit Parrot <bparrot@ti.com>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Jacek Anaszewski <jacek.anaszewski@gmail.com>,
Andrzej Pietrasiewicz <andrzej.p@samsung.com>,
Simon Horman <horms+renesas@verge.net.au>,
Tiffany Lin <tiffany.lin@mediatek.com>,
"moderated list:ARM/Mediatek SoC support"
<linux-arm-kernel@lists.infradead.org>,
Jacob chen <jacob2.chen@rock-chips.com>,
open list <linux-kernel@vger.kernel.org>,
Tomasz Figa <tfiga@chromium.org>,
"open list:MEDIA DRIVERS FOR RENESAS - FDP1"
<linux-renesas-soc@vger.kernel.org>,
Kyungmin Park <kyungmin.park@samsung.com>,
Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
Philipp Zabel <p.zabel@pengutronix.de>
Subject: Re: [PATCH] Remove deductively redundant NULL pointer checks
Date: Sun, 03 Mar 2019 20:34:09 -0300 [thread overview]
Message-ID: <1f7ab3d0a759c583fc33a83ee24ed780e40ce7a1.camel@collabora.com> (raw)
In-Reply-To: <1551228250-36426-1-git-send-email-shaobo@cs.utah.edu>
On Tue, 2019-02-26 at 17:43 -0700, Shaobo He wrote:
> The fixes included in this commit essentially removes NULL pointer
> checks on the return values of function `get_queue_ctx` as well as
> `v4l2_m2m_get_vq` defined in file v4l2-mem2mem.c.
>
> Function `get_queue_ctx` is very unlikely to return a NULL pointer
> because its return value is an address composed of the base address
> pointed by `m2m_ctx` and an offset of field `out_q_ctx` or `cap_q_ctx`.
> Since the offset of either field is not 0, for the return value to be
> NULL, pointer `m2m_ctx` must be a very large unsigned value such that
> its addition to the offset overflows to NULL which may be undefined
> according to this post:
> https://wdtz.org/catching-pointer-overflow-bugs.html. Moreover, even if
> `m2m_ctx` is NULL, the return value cannot be NULL, either. Therefore, I
> think it is reasonable to conclude that the return value of function
> `get_queue_ctx` cannot be NULL.
>
> Given the return values of `get_queue_ctx` not being NULL, we can follow
> a similar reasoning to conclude that the return value of
> `v4l2_mem_get_vq` cannot be NULL since its return value is the same
> address as the return value of `get_queue_ctx`. Therefore, this patch
> also removes NULL pointer checks on the return values of
> `v4l2_mem_get_vq`.
>
> Signed-off-by: Shaobo He <shaobo@cs.utah.edu>
Hi Shaobo,
It seems this is v2 of 1551128631-19713-1-git-send-email-shaobo@cs.utah.edu,
and it should be marked as such.
Do you think you can read Documentation/process/submitting-patches.rst,
for your future patches?
Also, two comments...
> drivers/media/platform/coda/coda-common.c | 4 ----
> drivers/media/platform/imx-pxp.c | 7 -------
> drivers/media/platform/m2m-deinterlace.c | 7 -------
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 7 -------
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 7 -------
> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 13 -------------
> drivers/media/platform/mx2_emmaprp.c | 7 -------
> drivers/media/platform/rcar_fdp1.c | 3 ---
> drivers/media/platform/rcar_jpu.c | 8 --------
> drivers/media/platform/rockchip/rga/rga.c | 4 ----
> drivers/media/platform/s5p-g2d/g2d.c | 4 ----
> drivers/media/platform/s5p-jpeg/jpeg-core.c | 7 -------
> drivers/media/platform/sh_veu.c | 2 --
> drivers/media/platform/ti-vpe/vpe.c | 7 -------
> drivers/media/platform/vicodec/vicodec-core.c | 5 -----
> drivers/media/platform/vim2m.c | 7 -------
> drivers/media/v4l2-core/v4l2-mem2mem.c | 4 ----
> 17 files changed, 103 deletions(-)
>
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 7518f01..ee1e05b 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -696,8 +696,6 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct v4l2_format *f,
> struct vb2_queue *vq;
>
> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> - if (!vq)
> - return -EINVAL;
>
> q_data = get_q_data(ctx, f->type);
> if (!q_data)
> @@ -817,8 +815,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
> ctx->quantization = f->fmt.pix.quantization;
>
> dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> - if (!dst_vq)
> - return -EINVAL;
>
> /*
> * Setting the capture queue format is not possible while the capture
> diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
> index c1c2554..d079b3c 100644
> --- a/drivers/media/platform/imx-pxp.c
> +++ b/drivers/media/platform/imx-pxp.c
> @@ -1071,13 +1071,8 @@ static int pxp_enum_fmt_vid_out(struct file *file, void *priv,
>
> static int pxp_g_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
> {
> - struct vb2_queue *vq;
> struct pxp_q_data *q_data;
>
> - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> - if (!vq)
> - return -EINVAL;
> -
It seems your patch also removes unused code, but this is not really explained in the
commit log.
Perhaps it is better to split all these changes on their own patch:
one patch to remove dead code, and then another patch to remove unneeded null checks.
And also, I think you should add some comments, either in v4l2_m2m_get_vq's declaration
or definition, explaining that the return value cannot be NULL.
I have to say: I'm not a fan of "improvement" patches in code paths that
are anything but hot... but knock yourself out!
Thanks,
Eze
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-03-03 23:34 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-27 0:43 [PATCH] Remove deductively redundant NULL pointer checks Shaobo He
2019-03-03 23:34 ` Ezequiel Garcia [this message]
-- strict thread matches above, loose matches on Subject: below --
2019-02-25 21:03 Shaobo He
2019-02-26 8:00 ` kbuild test robot
2019-02-26 8:47 ` kbuild test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1f7ab3d0a759c583fc33a83ee24ed780e40ce7a1.camel@collabora.com \
--to=ezequiel@collabora.com \
--cc=a.hajda@samsung.com \
--cc=andrew-ct.chen@mediatek.com \
--cc=andrzej.p@samsung.com \
--cc=bin.liu@mediatek.com \
--cc=bparrot@ti.com \
--cc=hans.verkuil@cisco.com \
--cc=heiko@sntech.de \
--cc=horms+renesas@verge.net.au \
--cc=jacek.anaszewski@gmail.com \
--cc=jacob2.chen@rock-chips.com \
--cc=kamil@wypas.org \
--cc=keescook@chromium.org \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=kuninori.morimoto.gx@renesas.com \
--cc=kyungmin.park@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=mikhail.ulyanov@cogentembedded.com \
--cc=p.zabel@pengutronix.de \
--cc=rick.chang@mediatek.com \
--cc=sakari.ailus@linux.intel.com \
--cc=scileont@gmail.com \
--cc=shaobo@cs.utah.edu \
--cc=tfiga@chromium.org \
--cc=tiffany.lin@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).