Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190118193200.32691-1-malat@debian.org>
In-Reply-To: <20190118193200.32691-1-malat@debian.org>
From:   Nick Desaulniers <nick.desaulniers@gmail.com>
Date:   Thu, 24 Jan 2019 20:26:24 -0800
Message-ID: <CAH7mPvi2TN0b=j-6fqDpjGPkZVSEQnDiT2nj9NWmoBOhUiwKEg@mail.gmail.com>
Subject: Re: [PATCH] writeback: tracing: Copy only up to 31 characters in
 strncpy() calls
To:     Mathieu Malaterre <malat@debian.org>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jan 18, 2019 at 11:32 AM Mathieu Malaterre <malat@debian.org> wrote:
>
> In the past an attempt was made to remove a set of warnings triggered by
> gcc 8.x and W=1 by changing calls to strncpy() into strlcpy(). This was
> rejected as one of the desired behavior is to keep initializing the rest
> of the destination string whenever the source string is less than the
> size.
>
> However the code makes it clear that this is not a desired behavior to
> copy the entire 32 characters into the destination buffer, since it is
> expected that the string is NUL terminated. So change the maximum number
> of characters to be copied to be the size of the destination buffer
> minus 1.
>
> Break long lines and make this patch go through `checkpatch --strict` with
> no errors.
>
> This commit removes the following warnings:
>
>   include/trace/events/writeback.h:69:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:99:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:179:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:223:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:277:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:299:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:324:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:375:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:586:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>   include/trace/events/writeback.h:660:3: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
>
> Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> Link: https://lore.kernel.org/patchwork/patch/910830/
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> ---
>  include/trace/events/writeback.h | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> index 32db72c7c055..7bc58980f84f 100644
> --- a/include/trace/events/writeback.h
> +++ b/include/trace/events/writeback.h
> @@ -67,7 +67,8 @@ TRACE_EVENT(writeback_dirty_page,
>
>         TP_fast_assign(
>                 strncpy(__entry->name,
> -                       mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)", 32);
> +                       mapping ? dev_name(inode_to_bdi(mapping->host)->dev) :
> +                       "(unknown)", sizeof(__entry->name) - 1);

Does strncpy guarantee that destination will be NULL terminated if
(sizeof(src) > sizeof(dst)) || (32 > sizeof(dst))?

>                 __entry->ino = mapping ? mapping->host->i_ino : 0;
>                 __entry->index = page->index;
>         ),
> @@ -97,7 +98,8 @@ DECLARE_EVENT_CLASS(writeback_dirty_inode_template,
>
>                 /* may be called for files on pseudo FSes w/ unregistered bdi */
>                 strncpy(__entry->name,
> -                       bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
> +                       bdi->dev ? dev_name(bdi->dev) : "(unknown)",
> +                       sizeof(__entry->name) - 1);
>                 __entry->ino            = inode->i_ino;
>                 __entry->state          = inode->i_state;
>                 __entry->flags          = flags;
> @@ -177,7 +179,8 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
>
>         TP_fast_assign(
>                 strncpy(__entry->name,
> -                       dev_name(inode_to_bdi(inode)->dev), 32);
> +                       dev_name(inode_to_bdi(inode)->dev),
> +                       sizeof(__entry->name) - 1);
>                 __entry->ino            = inode->i_ino;
>                 __entry->sync_mode      = wbc->sync_mode;
>                 __entry->cgroup_ino     = __trace_wbc_assign_cgroup(wbc);
> @@ -221,7 +224,8 @@ DECLARE_EVENT_CLASS(writeback_work_class,
>         ),
>         TP_fast_assign(
>                 strncpy(__entry->name,
> -                       wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)", 32);
> +                       wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)",
> +                       sizeof(__entry->name) - 1);
>                 __entry->nr_pages = work->nr_pages;
>                 __entry->sb_dev = work->sb ? work->sb->s_dev : 0;
>                 __entry->sync_mode = work->sync_mode;
> @@ -274,7 +278,8 @@ DECLARE_EVENT_CLASS(writeback_class,
>                 __field(unsigned int, cgroup_ino)
>         ),
>         TP_fast_assign(
> -               strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> +               strncpy(__entry->name, dev_name(wb->bdi->dev),
> +                       sizeof(__entry->name) - 1);
>                 __entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
>         ),
>         TP_printk("bdi %s: cgroup_ino=%u",
> @@ -296,7 +301,8 @@ TRACE_EVENT(writeback_bdi_register,
>                 __array(char, name, 32)
>         ),
>         TP_fast_assign(
> -               strncpy(__entry->name, dev_name(bdi->dev), 32);
> +               strncpy(__entry->name, dev_name(bdi->dev),
> +                       sizeof(__entry->name) - 1);
>         ),
>         TP_printk("bdi %s",
>                 __entry->name
> @@ -321,7 +327,8 @@ DECLARE_EVENT_CLASS(wbc_class,
>         ),
>
>         TP_fast_assign(
> -               strncpy(__entry->name, dev_name(bdi->dev), 32);
> +               strncpy(__entry->name, dev_name(bdi->dev),
> +                       sizeof(__entry->name) - 1);
>                 __entry->nr_to_write    = wbc->nr_to_write;
>                 __entry->pages_skipped  = wbc->pages_skipped;
>                 __entry->sync_mode      = wbc->sync_mode;
> @@ -372,7 +379,8 @@ TRACE_EVENT(writeback_queue_io,
>         ),
>         TP_fast_assign(
>                 unsigned long *older_than_this = work->older_than_this;
> -               strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> +               strncpy(__entry->name, dev_name(wb->bdi->dev),
> +                       sizeof(__entry->name) - 1);
>                 __entry->older  = older_than_this ?  *older_than_this : 0;
>                 __entry->age    = older_than_this ?
>                                   (jiffies - *older_than_this) * 1000 / HZ : -1;
> @@ -584,7 +592,8 @@ TRACE_EVENT(writeback_sb_inodes_requeue,
>
>         TP_fast_assign(
>                 strncpy(__entry->name,
> -                       dev_name(inode_to_bdi(inode)->dev), 32);
> +                       dev_name(inode_to_bdi(inode)->dev),
> +                       sizeof(__entry->name) - 1);
>                 __entry->ino            = inode->i_ino;
>                 __entry->state          = inode->i_state;
>                 __entry->dirtied_when   = inode->dirtied_when;
> @@ -658,7 +667,8 @@ DECLARE_EVENT_CLASS(writeback_single_inode_template,
>
>         TP_fast_assign(
>                 strncpy(__entry->name,
> -                       dev_name(inode_to_bdi(inode)->dev), 32);
> +                       dev_name(inode_to_bdi(inode)->dev),
> +                       sizeof(__entry->name) - 1);
>                 __entry->ino            = inode->i_ino;
>                 __entry->state          = inode->i_state;
>                 __entry->dirtied_when   = inode->dirtied_when;
> --
> 2.19.2
>