On 06/09/18 15:40, Patrick Bellasi wrote: > On 04-Sep 15:47, Juri Lelli wrote:
[...] > > Wondering if you want to fold the check below inside the > > > > if (user && !capable(CAP_SYS_NICE)) { > > ... > > } > > > > block. It would also save you from adding another parameter to the > > function. > > So, there are two reasons for that: > > 1) _I think_ we don't want to depend on capable(CAP_SYS_NICE) but > instead on capable(CAP_SYS_ADMIN) > > Does that make sense ? > > If yes, the I cannot fold it in the block you reported above > because we will not check for users with CAP_SYS_NICE. Ah, right, not sure though. Looks like CAP_SYS_NICE is used for settings that relates to priorities, affinity, etc.: CPU related stuff. Since here you are also dealing with something that seems to fall into the same realm, it might actually fit more than CAP_SYS_ADMIN? Now that I think more about it, would it actually make sense to allow unpriviledged users to lower their assigned umin/umax properties if they want? Something alike what happens for nice values or RT priorities. > 2) Then we could move it after that block, where there is another > set of checks with just: > > if (user) { > > We can potentially add the check there yes... but when uclamp is > not enabled we will still perform those checks or we have to add > some compiler guards... > > 3) ... or at least check for: > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP) > > Which is what I'm doing right after the block above (2). > > But, at this point, by passing in the parameter to the > __setscheduler_uclamp() call, I get the benefits of: > > a) reducing uclamp specific code in the caller > b) avoiding the checks on !CONFIG_UCLAMP_TASK build > > > > { > > > int group_id[UCLAMP_CNT] = { UCLAMP_NOT_VALID }; > > > int lower_bound, upper_bound; > > > struct uclamp_se *uc_se; > > > int result = 0; > > > > > > + if (!capable(CAP_SYS_ADMIN) && > > > + user && !uclamp_user_allowed) { > > > + return -EPERM; > > > + } > > > + > > Does all the above makes sense ? If we agree on CAP_SYS_ADMIN, however, your approach looks cleaner yes.