* [bug report] libnvdimm: fix mishandled nvdimm_clear_poison() return value
@ 2022-11-24 10:20 Dan Carpenter
2022-11-28 15:24 ` Jeff Moyer
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-11-24 10:20 UTC (permalink / raw)
To: dan.j.williams; +Cc: nvdimm
Hello Dan Williams,
The patch 868f036fee4b: "libnvdimm: fix mishandled
nvdimm_clear_poison() return value" from Dec 16, 2016, leads to the
following Smatch static checker warnings:
drivers/nvdimm/claim.c:287 nsio_rw_bytes() warn:
replace divide condition 'cleared / 512' with 'cleared >= 512'
drivers/nvdimm/bus.c:210 nvdimm_account_cleared_poison() warn:
replace divide condition 'cleared / 512' with 'cleared >= 512'
drivers/nvdimm/claim.c
252 static int nsio_rw_bytes(struct nd_namespace_common *ndns,
253 resource_size_t offset, void *buf, size_t size, int rw,
254 unsigned long flags)
255 {
256 struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
257 unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
258 sector_t sector = offset >> 9;
259 int rc = 0, ret = 0;
260
261 if (unlikely(!size))
262 return 0;
263
264 if (unlikely(offset + size > nsio->size)) {
265 dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n");
266 return -EFAULT;
267 }
268
269 if (rw == READ) {
270 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align)))
271 return -EIO;
272 if (copy_mc_to_kernel(buf, nsio->addr + offset, size) != 0)
273 return -EIO;
274 return 0;
275 }
276
277 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align))) {
278 if (IS_ALIGNED(offset, 512) && IS_ALIGNED(size, 512)
279 && !(flags & NVDIMM_IO_ATOMIC)) {
280 long cleared;
281
282 might_sleep();
283 cleared = nvdimm_clear_poison(&ndns->dev,
284 nsio->res.start + offset, size);
285 if (cleared < size)
286 rc = -EIO;
--> 287 if (cleared > 0 && cleared / 512) {
^^^^^^^^^^^^^
Smatch suggests changing this to "&& cleared >= 512" but it doesn't make
sense to say if (cleared > 0 && cleared >= 512) {. Probably what was
instead intended was "if (cleared > 0 && (cleared % 512) == 0) {"?
288 cleared /= 512;
289 badblocks_clear(&nsio->bb, sector, cleared);
290 }
291 arch_invalidate_pmem(nsio->addr + offset, size);
292 } else
293 rc = -EIO;
294 }
295
296 memcpy_flushcache(nsio->addr + offset, buf, size);
297 ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL);
298 if (ret)
299 rc = ret;
300
301 return rc;
302 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] libnvdimm: fix mishandled nvdimm_clear_poison() return value
2022-11-24 10:20 [bug report] libnvdimm: fix mishandled nvdimm_clear_poison() return value Dan Carpenter
@ 2022-11-28 15:24 ` Jeff Moyer
2022-11-29 5:22 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Moyer @ 2022-11-28 15:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dan.j.williams, nvdimm
Dan Carpenter <error27@gmail.com> writes:
> Hello Dan Williams,
>
> The patch 868f036fee4b: "libnvdimm: fix mishandled
> nvdimm_clear_poison() return value" from Dec 16, 2016, leads to the
> following Smatch static checker warnings:
>
> drivers/nvdimm/claim.c:287 nsio_rw_bytes() warn:
> replace divide condition 'cleared / 512' with 'cleared >= 512'
>
> drivers/nvdimm/bus.c:210 nvdimm_account_cleared_poison() warn:
> replace divide condition 'cleared / 512' with 'cleared >= 512'
>
> drivers/nvdimm/claim.c
> 252 static int nsio_rw_bytes(struct nd_namespace_common *ndns,
> 253 resource_size_t offset, void *buf, size_t size, int rw,
> 254 unsigned long flags)
> 255 {
> 256 struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
> 257 unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
> 258 sector_t sector = offset >> 9;
> 259 int rc = 0, ret = 0;
> 260
> 261 if (unlikely(!size))
> 262 return 0;
> 263
> 264 if (unlikely(offset + size > nsio->size)) {
> 265 dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n");
> 266 return -EFAULT;
> 267 }
> 268
> 269 if (rw == READ) {
> 270 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align)))
> 271 return -EIO;
> 272 if (copy_mc_to_kernel(buf, nsio->addr + offset, size) != 0)
> 273 return -EIO;
> 274 return 0;
> 275 }
> 276
> 277 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align))) {
> 278 if (IS_ALIGNED(offset, 512) && IS_ALIGNED(size, 512)
> 279 && !(flags & NVDIMM_IO_ATOMIC)) {
> 280 long cleared;
> 281
> 282 might_sleep();
> 283 cleared = nvdimm_clear_poison(&ndns->dev,
> 284 nsio->res.start + offset, size);
> 285 if (cleared < size)
> 286 rc = -EIO;
> --> 287 if (cleared > 0 && cleared / 512) {
> ^^^^^^^^^^^^^
> Smatch suggests changing this to "&& cleared >= 512" but it doesn't make
> sense to say if (cleared > 0 && cleared >= 512) {. Probably what was
> instead intended was "if (cleared > 0 && (cleared % 512) == 0) {"?
No, it is correct as written. cleared is the number of bytes cleared.
The badblocks_clear interface takes 512 byte sectors as an input. We
only want to call badblocks_clear if we cleared /at least/ one sector.
It could probably use a comment, though. :)
Cheers,
Jeff
>
> 288 cleared /= 512;
> 289 badblocks_clear(&nsio->bb, sector, cleared);
> 290 }
> 291 arch_invalidate_pmem(nsio->addr + offset, size);
> 292 } else
> 293 rc = -EIO;
> 294 }
> 295
> 296 memcpy_flushcache(nsio->addr + offset, buf, size);
> 297 ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL);
> 298 if (ret)
> 299 rc = ret;
> 300
> 301 return rc;
> 302 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] libnvdimm: fix mishandled nvdimm_clear_poison() return value
2022-11-28 15:24 ` Jeff Moyer
@ 2022-11-29 5:22 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2022-11-29 5:22 UTC (permalink / raw)
To: Jeff Moyer; +Cc: dan.j.williams, nvdimm
On Mon, Nov 28, 2022 at 10:24:07AM -0500, Jeff Moyer wrote:
> Dan Carpenter <error27@gmail.com> writes:
>
> > Hello Dan Williams,
> >
> > The patch 868f036fee4b: "libnvdimm: fix mishandled
> > nvdimm_clear_poison() return value" from Dec 16, 2016, leads to the
> > following Smatch static checker warnings:
> >
> > drivers/nvdimm/claim.c:287 nsio_rw_bytes() warn:
> > replace divide condition 'cleared / 512' with 'cleared >= 512'
> >
> > drivers/nvdimm/bus.c:210 nvdimm_account_cleared_poison() warn:
> > replace divide condition 'cleared / 512' with 'cleared >= 512'
> >
> > drivers/nvdimm/claim.c
> > 252 static int nsio_rw_bytes(struct nd_namespace_common *ndns,
> > 253 resource_size_t offset, void *buf, size_t size, int rw,
> > 254 unsigned long flags)
> > 255 {
> > 256 struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
> > 257 unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
> > 258 sector_t sector = offset >> 9;
> > 259 int rc = 0, ret = 0;
> > 260
> > 261 if (unlikely(!size))
> > 262 return 0;
> > 263
> > 264 if (unlikely(offset + size > nsio->size)) {
> > 265 dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n");
> > 266 return -EFAULT;
> > 267 }
> > 268
> > 269 if (rw == READ) {
> > 270 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align)))
> > 271 return -EIO;
> > 272 if (copy_mc_to_kernel(buf, nsio->addr + offset, size) != 0)
> > 273 return -EIO;
> > 274 return 0;
> > 275 }
> > 276
> > 277 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align))) {
> > 278 if (IS_ALIGNED(offset, 512) && IS_ALIGNED(size, 512)
> > 279 && !(flags & NVDIMM_IO_ATOMIC)) {
> > 280 long cleared;
> > 281
> > 282 might_sleep();
> > 283 cleared = nvdimm_clear_poison(&ndns->dev,
> > 284 nsio->res.start + offset, size);
> > 285 if (cleared < size)
> > 286 rc = -EIO;
> > --> 287 if (cleared > 0 && cleared / 512) {
> > ^^^^^^^^^^^^^
> > Smatch suggests changing this to "&& cleared >= 512" but it doesn't make
> > sense to say if (cleared > 0 && cleared >= 512) {. Probably what was
> > instead intended was "if (cleared > 0 && (cleared % 512) == 0) {"?
>
> No, it is correct as written. cleared is the number of bytes cleared.
> The badblocks_clear interface takes 512 byte sectors as an input. We
> only want to call badblocks_clear if we cleared /at least/ one sector.
>
> It could probably use a comment, though. :)
Okay. Thanks for looking at this!
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] libnvdimm: fix mishandled nvdimm_clear_poison() return value
@ 2023-07-08 8:40 Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-07-08 8:40 UTC (permalink / raw)
To: dan.j.williams; +Cc: nvdimm
Hello Dan Williams,
The patch 868f036fee4b: "libnvdimm: fix mishandled
nvdimm_clear_poison() return value" from Dec 16, 2016, leads to the
following Smatch static checker warning:
drivers/nvdimm/claim.c:285 nsio_rw_bytes()
warn: error code type promoted to positive: 'cleared'
drivers/nvdimm/claim.c
252 static int nsio_rw_bytes(struct nd_namespace_common *ndns,
253 resource_size_t offset, void *buf, size_t size, int rw,
254 unsigned long flags)
255 {
256 struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
257 unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
258 sector_t sector = offset >> 9;
259 int rc = 0, ret = 0;
260
261 if (unlikely(!size))
262 return 0;
263
264 if (unlikely(offset + size > nsio->size)) {
265 dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n");
266 return -EFAULT;
267 }
268
269 if (rw == READ) {
270 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align)))
271 return -EIO;
272 if (copy_mc_to_kernel(buf, nsio->addr + offset, size) != 0)
273 return -EIO;
274 return 0;
275 }
276
277 if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align))) {
278 if (IS_ALIGNED(offset, 512) && IS_ALIGNED(size, 512)
279 && !(flags & NVDIMM_IO_ATOMIC)) {
280 long cleared;
281
282 might_sleep();
283 cleared = nvdimm_clear_poison(&ndns->dev,
284 nsio->res.start + offset, size);
--> 285 if (cleared < size)
286 rc = -EIO;
cleared is long and size is unsigned long so negative error codes are
treated as success. We know that size is in increments of 512. Is size
== 0 and error? Maybe it should be:
if (cleared < 0)
rc = cleared;
else if (cleared == 0 || cleared < size)
rc = -EIO;
else
badblocks_clear(&nsio->bb, sector, cleared / 512);
arch_invalidate_pmem(nsio->addr + offset, size);
287 if (cleared > 0 && cleared / 512) {
288 cleared /= 512;
289 badblocks_clear(&nsio->bb, sector, cleared);
290 }
291 arch_invalidate_pmem(nsio->addr + offset, size);
292 } else
293 rc = -EIO;
294 }
295
296 memcpy_flushcache(nsio->addr + offset, buf, size);
297 ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL);
298 if (ret)
299 rc = ret;
300
301 return rc;
302 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-08 8:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-24 10:20 [bug report] libnvdimm: fix mishandled nvdimm_clear_poison() return value Dan Carpenter
2022-11-28 15:24 ` Jeff Moyer
2022-11-29 5:22 ` Dan Carpenter
2023-07-08 8:40 Dan Carpenter
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.