msr-safe
msr-safe copied to clipboard
LLNL
User provided ioctl() write mask is ignored
I believe this needs to be "&=" not "=" otherwise you are ignoring the user's request to only write a subset of the bits in the MSR.
https://github.com/LLNL/msr-safe/blob/b5bdf8b200db5a0bfa7e9ba2aadb85159f72c697/msr_batch.c#L124
As it is written, if someone provides a write mask in the ioctl structure that sets only the first 14 bits of the MSR_PKG_POWER_LIMIT msr, but the white list gives permission to write the entire register, then when they call into the batch ioctl all bits above bit 14 will be zeroed in the register.
How do we determine how many bits the user wishes to write? We allow for the user to be able to write a 0 or 1 to any of the bits defined in the write mask. If I recall correctly, there are not presently semantics for the user to say they only want to write the first 14 bits of the MSR.
I had misinterpreted the msr_batch_op.wmask:
https://github.com/LLNL/msr-safe/blob/b5bdf8b200db5a0bfa7e9ba2aadb85159f72c697/msr_safe.h#L53
as an input parameter to the ioctl. It says right there in the comment that it is an out parameter.
Can the implementation be changed so that this is an input parameter? Then you could just &= the user mask with the one from the whitelist before doing the read-modify-write in the driver. The call could set the msr_batch_op.err if the provided mask was not a subset of the whitelist mask.
The alternative would be user trying to do the read-modify-write with batch operations to the ioctl().
Something like the commit below should do the trick: https://github.com/cmcantalupo/msr-safe/commit/ba57451d70903c66e8a5aec43212e908ff5f142e
Overall, I like the proposed changes.
Modifying the wmask to be an IN parameter allows the user to specify exactly which bits they intend to write in the MSR and the proposed check will make sure that the user is allowed to change all of those bits.
@rountree, @slabasan, we need to take care that this change doesn't have unintended consequences on other applications that may be assuming the previous semantics. Do we know how many folks are using the batch interface for writing MSRs?
Also, how do we make sure that this is consistently carried out in the write() interface? The write() interface does not currently allow a means for specifying which bits are intended to be written.
In the case of the non-batched approach, it is quite feasible to do the read-modify-write in user space. (pread(), modify, pwrite()). Since the pwrite() interface does not allow for masking at a finer granularity than bytes, it is not really feasible to translate this feature to the pwrite() semantics.
I agree that we would need to leave the seek()/read()/write() mechanism as needing to interact with the entire MSR word. There is a side-effect in the current implementation that a user might assume they are able to write a certain set of bits when, in fact, they will only be allowed to alter a subset of those bits depending upon the whitelist mask.
Added a pull request here:
https://github.com/LLNL/msr-safe/pull/40
and I'm adding this patch to the OpenHPC distro of msr-safe.
