On 18 May 2015 at 18:14, Arnd Bergmann arnd@linaro.org wrote:
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
Thanks for your comments. And the simplification for the second definition of security_settime() is very reasonable, I'll simplify it. As well as modify the changelog to more helpful.