| Message ID | 20170125190443.20929-1-billodo@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 1/25/17 1:04 PM, Bill O'Donnell wrote: > From: Colin Ian King <colin.king@canonical.com> > > If pag cannot be allocated, the current error exit path will trip > a null pointer deference error when calling xfs_buf_hash_destroy > with a null pag. Fix this by adding a new error exit lable and > jumping to this, avoiding the hash destroy and unnecessary kmem_free > on pag. There are enough other changes that it probably needs a more accurate commit log and subject, and TBH probably switch Colin to reported-by. > Fixes CoverityScan CID#1397628 ("Dereference after null check") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > ------------ > v2: correct error exit in xfs_initialize_perag() to properly unwind > pags if error encountered. > > v3: correction to error case: ensure previous valid pags not torn > down and only new initialized pags are torn down. patch changelog goes under the "---" below, it shouldn't be part of the commitlog, FYI. > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > --- > fs/xfs/xfs_mount.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 9b9540d..afc49ac 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -188,8 +188,10 @@ xfs_initialize_perag( > { > xfs_agnumber_t index; > xfs_agnumber_t first_initialised = 0; > + xfs_agnumber_t next_agindex = 0; > xfs_perag_t *pag; > int error = -ENOMEM; > + int i; > > /* > * Walk the current per-ag tree so we don't try to initialise AGs > @@ -200,14 +202,15 @@ xfs_initialize_perag( > pag = xfs_perag_get(mp, index); > if (pag) { > xfs_perag_put(pag); > + next_agindex = index + 1; a little odd to keep incrementing next_agindex when we really only care that it's nonzero, right? > continue; > } > - if (!first_initialised) > + if (!first_initialised && (next_agindex > 0)) > first_initialised = index; ok, so we deem this index as first_initialized, but: > pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL); > if (!pag) > - goto out_unwind; > + goto out_unwind_pags; ...then it could fail. Which is why the unwind loop at the end still needs to check for (this) null pag, and skip if so. In fact if you ever get to that loop, I think it's guaranteed that the first time through will find the null pag for this index and skip it. It'd be a little nicer to move the first_initialized assignment down, after it's actually /been/ allocated and initialized. This patch also leaves this in place: if (xfs_buf_hash_init(pag)) goto out_unwind; ... out_unwind: xfs_buf_hash_destroy(pag); kmem_free(pag); so isn't this doing buf_hash_destroy on something that has failed to init in the first place? I have no idea if that's safe, but best to just be clean about it. In the big picture, there are 3 things happening in this loop that may need to be unwound for this and/or prior pags on an error: 1) pag memory allocation 2) xfs_buf_hash_init 3) radix_tree_insert For any given iteration through the loop, any of the above which succeed must be unwound for /this/ pag, and then all prior initialized pags must be unwound. So I'd expect something like this, though please check the details and adjust labels to your preference. :) bool first_init_done = false; for (index = 0; index < agcount; index++) { if (pag exists) continue; allocate(pag) if (failure) goto unwind_prior_pags; xfs_buf_hash_init(pag) if (failure) goto free_pag; radix_tree_insert(pag) if (failure) goto hash_destroy; /* this pag is fully initialized now */ if (!first_init_done) { first_initialized = index; first_init_done = true; } } /* unwind this pag on init failures */ hash_destroy: xfs_buf_hash_destroy(pag); free_pag: kmem_free(pag); unwind_prior_pags: /* back up to previous fully initialized pag and unwind it+prior */ index--; for (; index >= first_initialised; index--) pag = radix_tree_delete(index) xfs_buf_hash_destroy(pag) kmem_free(pag) > pag->pag_agno = index; > pag->pag_mount = mp; > spin_lock_init(&pag->pag_ici_lock); > @@ -242,8 +245,11 @@ xfs_initialize_perag( > out_unwind: > xfs_buf_hash_destroy(pag); > kmem_free(pag); > - for (; index > first_initialised; index--) { > - pag = radix_tree_delete(&mp->m_perag_tree, index); > +out_unwind_pags: > + for (i = index; i >= first_initialised; i--) { > + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i); (don't make i an int if it's really supposed to be an xfs_agnumber_t, which is unsigned) -Eric > + if (!pag) > + continue; > xfs_buf_hash_destroy(pag); > kmem_free(pag); > } > -- 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
On Thu, Jan 26, 2017 at 11:58:37AM -0600, Eric Sandeen wrote: > On 1/25/17 1:04 PM, Bill O'Donnell wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > If pag cannot be allocated, the current error exit path will trip > > a null pointer deference error when calling xfs_buf_hash_destroy > > with a null pag. Fix this by adding a new error exit lable and > > jumping to this, avoiding the hash destroy and unnecessary kmem_free > > on pag. > > There are enough other changes that it probably needs a more accurate > commit log and subject, and TBH probably switch Colin to reported-by. > > > Fixes CoverityScan CID#1397628 ("Dereference after null check") > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > ------------ > > v2: correct error exit in xfs_initialize_perag() to properly unwind > > pags if error encountered. > > > > v3: correction to error case: ensure previous valid pags not torn > > down and only new initialized pags are torn down. > > patch changelog goes under the "---" below, it shouldn't be part of the > commitlog, FYI. > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > --- > > fs/xfs/xfs_mount.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > index 9b9540d..afc49ac 100644 > > --- a/fs/xfs/xfs_mount.c > > +++ b/fs/xfs/xfs_mount.c > > @@ -188,8 +188,10 @@ xfs_initialize_perag( > > { > > xfs_agnumber_t index; > > xfs_agnumber_t first_initialised = 0; > > + xfs_agnumber_t next_agindex = 0; > > xfs_perag_t *pag; > > int error = -ENOMEM; > > + int i; > > > > /* > > * Walk the current per-ag tree so we don't try to initialise AGs > > @@ -200,14 +202,15 @@ xfs_initialize_perag( > > pag = xfs_perag_get(mp, index); > > if (pag) { > > xfs_perag_put(pag); > > + next_agindex = index + 1; > > a little odd to keep incrementing next_agindex when we really > only care that it's nonzero, right? It could just be a flag, but still would need to be checked if true/false. > > > continue; > > } > > - if (!first_initialised) > > + if (!first_initialised && (next_agindex > 0)) > > first_initialised = index; > > ok, so we deem this index as first_initialized, but: > > > pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL); > > if (!pag) > > - goto out_unwind; > > + goto out_unwind_pags; > > ...then it could fail. Which is why the unwind loop at the end still > needs to check for (this) null pag, and skip if so. In fact if you > ever get to that loop, I think it's guaranteed that the first time > through will find the null pag for this index and skip it. > > It'd be a little nicer to move the first_initialized assignment down, > after it's actually /been/ allocated and initialized. > > This patch also leaves this in place: > > if (xfs_buf_hash_init(pag)) > goto out_unwind; > ... > out_unwind: > xfs_buf_hash_destroy(pag); > kmem_free(pag); > > so isn't this doing buf_hash_destroy on something that has failed to > init in the first place? I have no idea if that's safe, but best to > just be clean about it. > > In the big picture, there are 3 things happening in this loop that may > need to be unwound for this and/or prior pags on an error: > > 1) pag memory allocation > 2) xfs_buf_hash_init > 3) radix_tree_insert > > For any given iteration through the loop, any of the above which succeed > must be unwound for /this/ pag, and then all prior initialized pags must > be unwound. So I'd expect something like this, though please check the > details and adjust labels to your preference. :) > > bool first_init_done = false; > > for (index = 0; index < agcount; index++) { > if (pag exists) > continue; > > allocate(pag) > if (failure) > goto unwind_prior_pags; > > xfs_buf_hash_init(pag) > if (failure) > goto free_pag; > > radix_tree_insert(pag) > if (failure) > goto hash_destroy; > > /* this pag is fully initialized now */ > if (!first_init_done) { > first_initialized = index; > first_init_done = true; > } > } > > /* unwind this pag on init failures */ > hash_destroy: > xfs_buf_hash_destroy(pag); > free_pag: > kmem_free(pag); > unwind_prior_pags: > /* back up to previous fully initialized pag and unwind it+prior */ > index--; > for (; index >= first_initialised; index--) > pag = radix_tree_delete(index) > xfs_buf_hash_destroy(pag) > kmem_free(pag) > > > > pag->pag_agno = index; > > pag->pag_mount = mp; > > spin_lock_init(&pag->pag_ici_lock); > > @@ -242,8 +245,11 @@ xfs_initialize_perag( > > out_unwind: > > xfs_buf_hash_destroy(pag); > > kmem_free(pag); > > - for (; index > first_initialised; index--) { > > - pag = radix_tree_delete(&mp->m_perag_tree, index); > > +out_unwind_pags: > > + for (i = index; i >= first_initialised; i--) { > > + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i); > > (don't make i an int if it's really supposed to be an xfs_agnumber_t, > which is unsigned) It might be wrong, but I used an int in order to handle the case where first_initialized==0, and a decrement of an unsigned would roll over instead of going negative (so i >= first_initialized would be true again). > > -Eric > > > + if (!pag) > > + continue; > > xfs_buf_hash_destroy(pag); > > kmem_free(pag); > > } > > -- 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
On 1/26/17 12:55 PM, Bill O'Donnell wrote: >>> @@ -242,8 +245,11 @@ xfs_initialize_perag( >>> out_unwind: >>> xfs_buf_hash_destroy(pag); >>> kmem_free(pag); >>> - for (; index > first_initialised; index--) { >>> - pag = radix_tree_delete(&mp->m_perag_tree, index); >>> +out_unwind_pags: >>> + for (i = index; i >= first_initialised; i--) { >>> + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i); >> (don't make i an int if it's really supposed to be an xfs_agnumber_t, >> which is unsigned) > It might be wrong, but I used an int in order to handle the case where > first_initialized==0, and a decrement of an unsigned would roll over > instead of going negative (so i >= first_initialized would be true again). Oh, urgh. I see. OTOH AG count is unsigned, so if you have index > 2147483648 AGs, your "i >= first" will fail because i will be negative in /that/ comparison. Ok, how about something like: out_unwind_pags: /* start unwinding at previous index */ index--; do { /* uninitialize pag at this index */ index--; } while (index && index >= first_initialised); -Eric -- 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
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 9b9540d..afc49ac 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -188,8 +188,10 @@ xfs_initialize_perag( { xfs_agnumber_t index; xfs_agnumber_t first_initialised = 0; + xfs_agnumber_t next_agindex = 0; xfs_perag_t *pag; int error = -ENOMEM; + int i; /* * Walk the current per-ag tree so we don't try to initialise AGs @@ -200,14 +202,15 @@ xfs_initialize_perag( pag = xfs_perag_get(mp, index); if (pag) { xfs_perag_put(pag); + next_agindex = index + 1; continue; } - if (!first_initialised) + if (!first_initialised && (next_agindex > 0)) first_initialised = index; pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL); if (!pag) - goto out_unwind; + goto out_unwind_pags; pag->pag_agno = index; pag->pag_mount = mp; spin_lock_init(&pag->pag_ici_lock); @@ -242,8 +245,11 @@ xfs_initialize_perag( out_unwind: xfs_buf_hash_destroy(pag); kmem_free(pag); - for (; index > first_initialised; index--) { - pag = radix_tree_delete(&mp->m_perag_tree, index); +out_unwind_pags: + for (i = index; i >= first_initialised; i--) { + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i); + if (!pag) + continue; xfs_buf_hash_destroy(pag); kmem_free(pag); }