On Thursday 14 May 2015 15:03:10 Baolin Wang wrote:
> This patch changes the "settime" pointer's argument with timespec64 type in
> security_operations
> structure, and also introduces the security_settime64() function with
> timespec64 type, meanwhile
> moves the security_settime() defined in security/security.c file to
> include/linux/security.h file
> as a static function.
>
> Also introduces a default security_settime64() function with timespec64 type
> when it doesn't define
> the CONFIG_SECURITY macro. And change the cap_settime() function's argument
> with timespec64 type,
> which is only used by security_settime64() function.
Regarding the changelog: please start each patch changelog with a
description of the current behavior and what is wrong with it, only
then explain what you are doing to address it. You description above
only says what you do, not why, and that is not too helpful for someone
trying to dig through the git history later, or trying to review the
patch out of context, for that matter.
Also, watch out for overly long lines. Try to make the subject no more
than 60 characters (as explained before), and limit the text lines to
about 70 characters.
The patch itself looks good to me, just one small improvement I'd do:
> @@ -1790,7 +1791,13 @@ int security_capable_noaudit(const struct cred *cred,
> struct user_namespace *ns,
> int security_quotactl(int cmds, int type, int id, struct super_block *sb);
> int security_quota_on(struct dentry *dentry);
> int security_syslog(int type);
> -int security_settime(const struct timespec *ts, const struct timezone *tz);
> +int security_settime64(const struct timespec64 *ts, const struct timezone
> *tz);
> +static int security_settime(const struct timespec *ts, const struct timezone
> *tz)
> +{
> + struct timespec64 ts64 = timespec_to_timespec64(*ts);
> +
> + return security_settime64(&ts64, tz);
> +}
> int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
> int security_bprm_set_creds(struct linux_binprm *bprm);
> int security_bprm_check(struct linux_binprm *bprm);
> @@ -2040,10 +2047,18 @@ static inline int security_syslog(int type)
> return 0;
> }
>
> +static inline int security_settime64(const struct timespec64 *ts,
> + const struct timezone *tz)
> +{
> + return cap_settime(ts, tz);
> +}
> +
> static inline int security_settime(const struct timespec *ts,
> const struct timezone *tz)
> {
> - return cap_settime(ts, tz);
> + struct timsepc64 ts64 = timespec_to_timespec64(*ts);
> +
> + return security_settime64(&ts64, tz);
> }
>
> static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long
> pages)
You have added two identical definitions for security_settime(). I would
try to move that definition outside the #ifdef so you only need one of the
two. If that ends up being ugly, I'd simplify the second definition to
just call cap_settime() directly: security_settime64() is only a trivial
wrapper here, so you can do the cap_settime() call in security_settime()
and avoid the extra indirection (though the compiler won't care).
Arnd
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
