* [bug report] xfs: remove dfops param from internal bmap extent helpers
@ 2018-07-19 8:12 Dan Carpenter
2018-07-19 10:51 ` Brian Foster
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2018-07-19 8:12 UTC (permalink / raw)
To: bfoster; +Cc: linux-xfs
Hello Brian Foster,
The patch 81ba8f3e947c: "xfs: remove dfops param from internal bmap
extent helpers" from Jul 11, 2018, leads to the following static
checker warning:
fs/xfs/libxfs/xfs_bmap.c:2465 xfs_bmap_add_extent_unwritten_real()
error: we previously assumed 'tp' could be null (see line 2037)
fs/xfs/libxfs/xfs_bmap.c
2017 xfs_bmap_add_extent_unwritten_real(
2018 struct xfs_trans *tp,
2019 xfs_inode_t *ip, /* incore inode pointer */
2020 int whichfork,
2021 struct xfs_iext_cursor *icur,
2022 xfs_btree_cur_t **curp, /* if *curp is null, not a btree */
2023 xfs_bmbt_irec_t *new, /* new data to add to file extents */
2024 int *logflagsp) /* inode logging flags */
2025 {
2026 xfs_btree_cur_t *cur; /* btree cursor */
2027 int error; /* error return value */
2028 int i; /* temp state */
2029 xfs_ifork_t *ifp; /* inode fork pointer */
2030 xfs_fileoff_t new_endoff; /* end offset of new entry */
2031 xfs_bmbt_irec_t r[3]; /* neighbor extent entries */
2032 /* left is 0, right is 1, prev is 2 */
2033 int rval=0; /* return value (logging flags) */
2034 int state = xfs_bmap_fork_to_state(whichfork);
2035 struct xfs_mount *mp = ip->i_mount;
2036 struct xfs_bmbt_irec old;
2037 struct xfs_defer_ops *dfops = tp ? tp->t_dfops : NULL;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The patch adds a check for NULL.
2038
2039 *logflagsp = 0;
2040
[ snip ]
2454
2455 /* update reverse mappings */
2456 error = xfs_rmap_convert_extent(mp, dfops, ip, whichfork, new);
2457 if (error)
2458 goto done;
2459
2460 /* convert to a btree if necessary */
2461 if (xfs_bmap_needs_btree(ip, whichfork)) {
2462 int tmp_logflags; /* partial log flag return val */
2463
2464 ASSERT(cur == NULL);
2465 error = xfs_bmap_extents_to_btree(tp, ip, &cur, 0,
^^
Existing code dereferences "tp" without checking for NULL.
2466 &tmp_logflags, whichfork);
2467 *logflagsp |= tmp_logflags;
2468 if (error)
2469 goto done;
2470 }
2471
See also:
fs/xfs/libxfs/xfs_bmap.c:4376 xfs_bmapi_write() error: we previously assumed 'tp' could be null (see line 4272)
fs/xfs/libxfs/xfs_bmap.c:4890 xfs_bmap_del_extent_real() error: we previously assumed 'tp' could be null (see line 4866)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] xfs: remove dfops param from internal bmap extent helpers
2018-07-19 8:12 [bug report] xfs: remove dfops param from internal bmap extent helpers Dan Carpenter
@ 2018-07-19 10:51 ` Brian Foster
2018-07-19 11:01 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Brian Foster @ 2018-07-19 10:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-xfs
On Thu, Jul 19, 2018 at 11:12:32AM +0300, Dan Carpenter wrote:
> Hello Brian Foster,
>
> The patch 81ba8f3e947c: "xfs: remove dfops param from internal bmap
> extent helpers" from Jul 11, 2018, leads to the following static
> checker warning:
>
> fs/xfs/libxfs/xfs_bmap.c:2465 xfs_bmap_add_extent_unwritten_real()
> error: we previously assumed 'tp' could be null (see line 2037)
>
> fs/xfs/libxfs/xfs_bmap.c
> 2017 xfs_bmap_add_extent_unwritten_real(
> 2018 struct xfs_trans *tp,
> 2019 xfs_inode_t *ip, /* incore inode pointer */
> 2020 int whichfork,
> 2021 struct xfs_iext_cursor *icur,
> 2022 xfs_btree_cur_t **curp, /* if *curp is null, not a btree */
> 2023 xfs_bmbt_irec_t *new, /* new data to add to file extents */
> 2024 int *logflagsp) /* inode logging flags */
> 2025 {
> 2026 xfs_btree_cur_t *cur; /* btree cursor */
> 2027 int error; /* error return value */
> 2028 int i; /* temp state */
> 2029 xfs_ifork_t *ifp; /* inode fork pointer */
> 2030 xfs_fileoff_t new_endoff; /* end offset of new entry */
> 2031 xfs_bmbt_irec_t r[3]; /* neighbor extent entries */
> 2032 /* left is 0, right is 1, prev is 2 */
> 2033 int rval=0; /* return value (logging flags) */
> 2034 int state = xfs_bmap_fork_to_state(whichfork);
> 2035 struct xfs_mount *mp = ip->i_mount;
> 2036 struct xfs_bmbt_irec old;
> 2037 struct xfs_defer_ops *dfops = tp ? tp->t_dfops : NULL;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The patch adds a check for NULL.
>
IIRC, the NULL check was necessary here to cover the reflink unwritten
extent conversion callers that pass a NULL transaction. So a NULL tp
only happens when whichfork == XFS_COW_FORK...
> 2038
> 2039 *logflagsp = 0;
> 2040
>
> [ snip ]
>
> 2454
> 2455 /* update reverse mappings */
> 2456 error = xfs_rmap_convert_extent(mp, dfops, ip, whichfork, new);
> 2457 if (error)
> 2458 goto done;
> 2459
> 2460 /* convert to a btree if necessary */
> 2461 if (xfs_bmap_needs_btree(ip, whichfork)) {
> 2462 int tmp_logflags; /* partial log flag return val */
> 2463
> 2464 ASSERT(cur == NULL);
> 2465 error = xfs_bmap_extents_to_btree(tp, ip, &cur, 0,
> ^^
> Existing code dereferences "tp" without checking for NULL.
>
... and we only get here when xfs_bmap_needs_btree() is true, which
requires whichfork != XFS_COW_FORK (because I don't think such "convert
only" cow fork changes are ever logged).
So I don't think this patch changes behavior in any way and technically
this is a false positive. That said, I'm not opposed to tweaking the
function-local logic if it facilitates static checking (and clarity,
more importantly).
It sounds to me that adding a '... && tp' check to a few of these spots
may quiet the checker, I'm just not sure how to verify. Can this be run
locally or triggered somehow to verify a potential fix?
Brian
> 2466 &tmp_logflags, whichfork);
> 2467 *logflagsp |= tmp_logflags;
> 2468 if (error)
> 2469 goto done;
> 2470 }
> 2471
>
> See also:
>
> fs/xfs/libxfs/xfs_bmap.c:4376 xfs_bmapi_write() error: we previously assumed 'tp' could be null (see line 4272)
> fs/xfs/libxfs/xfs_bmap.c:4890 xfs_bmap_del_extent_real() error: we previously assumed 'tp' could be null (see line 4866)
>
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] xfs: remove dfops param from internal bmap extent helpers
2018-07-19 10:51 ` Brian Foster
@ 2018-07-19 11:01 ` Dan Carpenter
2018-07-19 11:26 ` Brian Foster
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2018-07-19 11:01 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
Thanks for looking at this. I suspected it might be something like this
so I would only send an email like this when the code was very recent
so hopefully you would know the answers off the top of your head so it
wasn't too much bother.
On Thu, Jul 19, 2018 at 06:51:02AM -0400, Brian Foster wrote:
> > 2455 /* update reverse mappings */
> > 2456 error = xfs_rmap_convert_extent(mp, dfops, ip, whichfork, new);
> > 2457 if (error)
> > 2458 goto done;
> > 2459
> > 2460 /* convert to a btree if necessary */
> > 2461 if (xfs_bmap_needs_btree(ip, whichfork)) {
> > 2462 int tmp_logflags; /* partial log flag return val */
> > 2463
> > 2464 ASSERT(cur == NULL);
> > 2465 error = xfs_bmap_extents_to_btree(tp, ip, &cur, 0,
> > ^^
> > Existing code dereferences "tp" without checking for NULL.
> >
>
> ... and we only get here when xfs_bmap_needs_btree() is true, which
> requires whichfork != XFS_COW_FORK (because I don't think such "convert
> only" cow fork changes are ever logged).
>
> So I don't think this patch changes behavior in any way and technically
> this is a false positive. That said, I'm not opposed to tweaking the
> function-local logic if it facilitates static checking (and clarity,
> more importantly).
>
> It sounds to me that adding a '... && tp' check to a few of these spots
> may quiet the checker, I'm just not sure how to verify. Can this be run
> locally or triggered somehow to verify a potential fix?
That would silence the warning but I think the code is probably more
readable as-is. There is some more logic that I have been needing to
add to Smatch which might help here... I just haven't got around to it
yet. I think we just leave it as-is and re-think in a few years if the
warning is still around.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] xfs: remove dfops param from internal bmap extent helpers
2018-07-19 11:01 ` Dan Carpenter
@ 2018-07-19 11:26 ` Brian Foster
0 siblings, 0 replies; 4+ messages in thread
From: Brian Foster @ 2018-07-19 11:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-xfs
On Thu, Jul 19, 2018 at 02:01:31PM +0300, Dan Carpenter wrote:
> Thanks for looking at this. I suspected it might be something like this
> so I would only send an email like this when the code was very recent
> so hopefully you would know the answers off the top of your head so it
> wasn't too much bother.
>
No problem..
> On Thu, Jul 19, 2018 at 06:51:02AM -0400, Brian Foster wrote:
> > > 2455 /* update reverse mappings */
> > > 2456 error = xfs_rmap_convert_extent(mp, dfops, ip, whichfork, new);
> > > 2457 if (error)
> > > 2458 goto done;
> > > 2459
> > > 2460 /* convert to a btree if necessary */
> > > 2461 if (xfs_bmap_needs_btree(ip, whichfork)) {
> > > 2462 int tmp_logflags; /* partial log flag return val */
> > > 2463
> > > 2464 ASSERT(cur == NULL);
> > > 2465 error = xfs_bmap_extents_to_btree(tp, ip, &cur, 0,
> > > ^^
> > > Existing code dereferences "tp" without checking for NULL.
> > >
> >
> > ... and we only get here when xfs_bmap_needs_btree() is true, which
> > requires whichfork != XFS_COW_FORK (because I don't think such "convert
> > only" cow fork changes are ever logged).
> >
> > So I don't think this patch changes behavior in any way and technically
> > this is a false positive. That said, I'm not opposed to tweaking the
> > function-local logic if it facilitates static checking (and clarity,
> > more importantly).
> >
> > It sounds to me that adding a '... && tp' check to a few of these spots
> > may quiet the checker, I'm just not sure how to verify. Can this be run
> > locally or triggered somehow to verify a potential fix?
>
> That would silence the warning but I think the code is probably more
> readable as-is. There is some more logic that I have been needing to
> add to Smatch which might help here... I just haven't got around to it
> yet. I think we just leave it as-is and re-think in a few years if the
> warning is still around.
>
Yeah, I wasn't totally convinced that actually addressed the clarity
part, that was just the easiest thing to test. ;) I'm fine with leaving
it as is if the checker can (or will) deal with it (and nobody else
complains). Thanks.
Brian
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-07-19 12:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-19 8:12 [bug report] xfs: remove dfops param from internal bmap extent helpers Dan Carpenter
2018-07-19 10:51 ` Brian Foster
2018-07-19 11:01 ` Dan Carpenter
2018-07-19 11:26 ` Brian Foster
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.