imatrix: fix oob writes if src1 is not contiguous #13286
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CISC
reviewed
May 3, 2025
| @@ -180,7 +182,7 @@ bool IMatrixCollector::collect_imatrix(struct ggml_tensor * t, bool ask, void * | |||
| ++e.ncall; | |||
| LOG_DBGV(2, "%s[%d]: %32s, %s, %5d x %5d, %d\n", __func__, m_last_call, wname.c_str(), ggml_op_name(t->op), (int)src1->ne[0], (int)src1->ne[1], (int)src1->type); | |||
| for (int row = 0; row < (int)src1->ne[1]; ++row) { | |||
| const float * x = data + row * src1->ne[0]; | |||
| const float * x = (const float *) (data + row * src1->nb[1]); | |||
There was a problem hiding this comment.
Excuse my ignorance if I'm asking silly questions, but why nb[1] instead of nb[0]?
There was a problem hiding this comment.
nb[0] is the byte offset when changing dimension 0 by 1, nb[1] is the byte offset when changing dimension 1 by 1.
slaren
approved these changes
May 3, 2025
|
@JohannesGaessler In order for you not to have to look at the above linked PR, the solution is to loop over each attention head and compute and store them consecutively, meaning that values/counts must be resized to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The imatrix code implicitly assumes that
src1is contiguous when copying data from a backend to host memory. As a result the vector to which the data is written can end up being resized to a size that is smaller than the amount of data thatggml_backend_tensor_getwrites, resulting in out-of-bounds writes.This PR makes it so that the host buffer always has the exact same size as the amount of data that is being copied. Also, if
src1is not contiguous, then this is considered for calculating the byte addresses of matrix rows.Unless I'm misunderstanding the code the cases
ne12 > 1andne13 > 1are also going to result in unexpected behavior but I don't know what the correct fix would be.