From: liaoweixiong <liaoweixiong@allwinnertech.com>
To: Dan Carpenter <dan.carpenter@oracle.com>, kbuild@01.org
Cc: kbuild-all@01.org, Kees Cook <keescook@chromium.org>,
Anton Vorontsov <anton@enomsg.org>,
Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
Jonathan Corbet <corbet@lwn.net>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Nicolas Ferre <nicolas.ferre@microchip.com>,
Arnd Bergmann <arnd@arndb.de>, Rob Herring <robh@kernel.org>,
Randy Dunlap <rdunlap@infradead.org>,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v12 1/4] pstore/blk: new support logger for block devices
Date: Tue, 5 Mar 2019 20:29:35 +0800 [thread overview]
Message-ID: <d3d8621b-9d12-d03a-5f2a-76c0d8c464db@allwinnertech.com> (raw)
In-Reply-To: <20190305071242.GF13452@kadam>
hi Dan Carpenter,
On 2019/03/05 15:12, Dan Carpenter wrote:
> Hi liaoweixiong,
>
> url: https://github.com/0day-ci/linux/commits/liaoweixiong/pstore-block-new-support-logger-for-block-devices/20190303-142003
> base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
>
> smatch warnings:
> fs/pstore/blkzone.c:180 blkz_zone_write() error: we previously assumed 'zone->buffer' could be null (see line 167)
>
> # https://github.com/0day-ci/linux/commit/113727d0f1946ad094dbc6531d653a88c7a221bf
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 113727d0f1946ad094dbc6531d653a88c7a221bf
> vim +180 fs/pstore/blkzone.c
>
> 113727d0 liaoweixiong 2019-02-28 153
> 113727d0 liaoweixiong 2019-02-28 154 static int blkz_zone_write(struct blkz_zone *zone,
> 113727d0 liaoweixiong 2019-02-28 155 enum blkz_flush_mode flush_mode, const char *buf,
> 113727d0 liaoweixiong 2019-02-28 156 size_t len, unsigned long off)
> 113727d0 liaoweixiong 2019-02-28 157 {
> 113727d0 liaoweixiong 2019-02-28 158 struct blkz_info *info = blkz_cxt.bzinfo;
> 113727d0 liaoweixiong 2019-02-28 159 ssize_t wcnt;
> 113727d0 liaoweixiong 2019-02-28 160 ssize_t (*writeop)(const char *buf, size_t bytes, loff_t pos);
> 113727d0 liaoweixiong 2019-02-28 161 size_t wlen;
> 113727d0 liaoweixiong 2019-02-28 162
> 113727d0 liaoweixiong 2019-02-28 163 if (off > zone->buffer_size)
> 113727d0 liaoweixiong 2019-02-28 164 return -EINVAL;
> 113727d0 liaoweixiong 2019-02-28 165 wlen = min_t(size_t, len, zone->buffer_size - off);
> 113727d0 liaoweixiong 2019-02-28 166 if (flush_mode != FLUSH_META && flush_mode != FLUSH_NONE) {
> 113727d0 liaoweixiong 2019-02-28 @167 if (buf && zone->buffer)
> ^^^^^^^^^^^^
> Check.
>
zone->buffer should not be checked whether null as it will never be null
here. I will fix it on next version.
zone->buffer was allocated when the zone was initialized (see line 995).
Pstore/blk will not go on if allocates buffer for zone->buffer failed.
> 113727d0 liaoweixiong 2019-02-28 168 memcpy(zone->buffer->data + off, buf, wlen);
> 113727d0 liaoweixiong 2019-02-28 169 atomic_set(&zone->buffer->datalen, wlen + off);
> 113727d0 liaoweixiong 2019-02-28 170 }
> 113727d0 liaoweixiong 2019-02-28 171
> 113727d0 liaoweixiong 2019-02-28 172 writeop = is_on_panic() ? info->panic_write : info->write;
> 113727d0 liaoweixiong 2019-02-28 173 if (!writeop)
> 113727d0 liaoweixiong 2019-02-28 174 return -EINVAL;
> 113727d0 liaoweixiong 2019-02-28 175
> 113727d0 liaoweixiong 2019-02-28 176 switch (flush_mode) {
> 113727d0 liaoweixiong 2019-02-28 177 case FLUSH_NONE:
> 113727d0 liaoweixiong 2019-02-28 178 return 0;
> 113727d0 liaoweixiong 2019-02-28 179 case FLUSH_PART:
> 113727d0 liaoweixiong 2019-02-28 @180 wcnt = writeop((const char *)zone->buffer->data + off, wlen,
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Unchecked.
>
> 113727d0 liaoweixiong 2019-02-28 181 zone->off + sizeof(*zone->buffer) + off);
> ^^^^^^^^^^^^
> This is weird. I can't fetch for-next/pstore so I don't know what
> type "buffer" is. It's vague. We also have ->buffer_size which seems
> like a more expected way to describe the size.
>
The type of buffer is struct blkz_buffer (see line 98). struct
blkz_buffer is a header of data, who's member data[0] point to real
data. The codes "sizeof(*zone->buffer)" just to get size of header.
There is a size recorder for blkz_buffer->data on struct blkz_zone. It
is no need to write to block device, that's why it do not live in struct
blkz_buffer.
> 113727d0 liaoweixiong 2019-02-28 182 if (wcnt != wlen)
> 113727d0 liaoweixiong 2019-02-28 183 goto set_dirty;
> 113727d0 liaoweixiong 2019-02-28 184 case FLUSH_META:
> 113727d0 liaoweixiong 2019-02-28 185 wlen = sizeof(struct blkz_buffer);
> 113727d0 liaoweixiong 2019-02-28 186 wcnt = writeop((const char *)zone->buffer, wlen, zone->off);
> 113727d0 liaoweixiong 2019-02-28 187 if (wcnt != wlen)
> 113727d0 liaoweixiong 2019-02-28 188 goto set_dirty;
> 113727d0 liaoweixiong 2019-02-28 189 break;
> 113727d0 liaoweixiong 2019-02-28 190 case FLUSH_ALL:
> 113727d0 liaoweixiong 2019-02-28 191 wlen = buffer_datalen(zone) + sizeof(*zone->buffer);
> 113727d0 liaoweixiong 2019-02-28 192 wcnt = writeop((const char *)zone->buffer, wlen, zone->off);
> 113727d0 liaoweixiong 2019-02-28 193 if (wcnt != wlen)
> 113727d0 liaoweixiong 2019-02-28 194 goto set_dirty;
> 113727d0 liaoweixiong 2019-02-28 195 break;
> 113727d0 liaoweixiong 2019-02-28 196 }
> 113727d0 liaoweixiong 2019-02-28 197
> 113727d0 liaoweixiong 2019-02-28 198 return 0;
> 113727d0 liaoweixiong 2019-02-28 199 set_dirty:
> 113727d0 liaoweixiong 2019-02-28 200 pr_err("write failed with %zd returned, set dirty\n", wcnt);
> 113727d0 liaoweixiong 2019-02-28 201 atomic_set(&zone->dirty, true);
> 113727d0 liaoweixiong 2019-02-28 202 return -EBUSY;
> 113727d0 liaoweixiong 2019-02-28 203 }
> 113727d0 liaoweixiong 2019-02-28 204
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
--
liaoweixiong
next prev parent reply other threads:[~2019-03-05 12:29 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-28 7:12 [PATCH v12 0/4] pstore/block: new support logger for block devices liaoweixiong
2019-02-28 7:12 ` [PATCH v12 1/4] pstore/blk: " liaoweixiong
2019-03-05 7:12 ` Dan Carpenter
2019-03-05 12:29 ` liaoweixiong [this message]
2019-02-28 7:12 ` [PATCH v12 2/4] pstore/blk: add blkoops for pstore_blk liaoweixiong
2019-03-06 1:14 ` Randy Dunlap
2019-03-07 1:01 ` liaoweixiong
2019-02-28 7:12 ` [PATCH v12 3/4] pstore/blk: support pmsg for pstore block liaoweixiong
2019-03-06 1:16 ` Randy Dunlap
2019-03-07 1:05 ` liaoweixiong
2019-02-28 7:12 ` [PATCH v12 4/4] Documentation: pstore/blk: create document for pstore_blk liaoweixiong
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=d3d8621b-9d12-d03a-5f2a-76c0d8c464db@allwinnertech.com \
--to=liaoweixiong@allwinnertech.com \
--cc=anton@enomsg.org \
--cc=arnd@arndb.de \
--cc=ccross@android.com \
--cc=corbet@lwn.net \
--cc=dan.carpenter@oracle.com \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=kbuild-all@01.org \
--cc=kbuild@01.org \
--cc=keescook@chromium.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mchehab+samsung@kernel.org \
--cc=nicolas.ferre@microchip.com \
--cc=rdunlap@infradead.org \
--cc=robh@kernel.org \
--cc=tony.luck@intel.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.