Commit 7d7f315b authored by Paul Mackerras's avatar Paul Mackerras

PPC32: Start adding __user to mark pointers from userspace.

This reduces the number of warnings from Linus' `check' program
for stuff in arch/ppc.
parent 5bbcb162
......@@ -189,7 +189,7 @@ fix_alignment(struct pt_regs *regs)
#endif
int i, t;
int reg, areg;
unsigned char *addr;
unsigned char __user *addr;
union {
long l;
float f;
......@@ -252,7 +252,7 @@ fix_alignment(struct pt_regs *regs)
* pt_regs structure is overloaded and is really from the DEAR.
*/
addr = (unsigned char *)regs->dar;
addr = (unsigned char __user *)regs->dar;
/* Verify the address of the operand */
if (user_mode(regs)) {
......
......@@ -584,7 +584,7 @@ static int irq_affinity_read_proc (char *page, char **start, off_t off,
return sprintf (page, "%08x\n", irq_affinity[(int)data]);
}
static unsigned int parse_hex_value (const char *buffer,
static unsigned int parse_hex_value (const char __user *buffer,
unsigned long count, unsigned long *ret)
{
unsigned char hexnum [HEX_DIGITS];
......@@ -621,7 +621,7 @@ static unsigned int parse_hex_value (const char *buffer,
return 0;
}
static int irq_affinity_write_proc (struct file *file, const char *buffer,
static int irq_affinity_write_proc (struct file *file, const char __user *buffer,
unsigned long count, void *data)
{
int irq = (int) data, full_count = count, err;
......@@ -660,7 +660,7 @@ static int prof_cpu_mask_read_proc (char *page, char **start, off_t off,
return sprintf (page, "%08lx\n", *mask);
}
static int prof_cpu_mask_write_proc (struct file *file, const char *buffer,
static int prof_cpu_mask_write_proc (struct file *file, const char __user *buffer,
unsigned long count, void *data)
{
unsigned long *mask = (unsigned long *) data, full_count = count, err;
......
......@@ -30,13 +30,13 @@
#include <asm/cputable.h>
#include <asm/system.h>
static ssize_t ppc_htab_read(struct file * file, char * buf,
static ssize_t ppc_htab_read(struct file * file, char __user * buf,
size_t count, loff_t *ppos);
static ssize_t ppc_htab_write(struct file * file, const char * buffer,
static ssize_t ppc_htab_write(struct file * file, const char __user * buffer,
size_t count, loff_t *ppos);
static long long ppc_htab_lseek(struct file * file, loff_t offset, int orig);
int proc_dol2crvec(ctl_table *table, int write, struct file *filp,
void *buffer, size_t *lenp);
void __user *buffer, size_t *lenp);
extern PTE *Hash, *Hash_end;
extern unsigned long Hash_size, Hash_mask;
......@@ -109,7 +109,7 @@ static char *pmc2_lookup(unsigned long mmcr0)
* is _REALLY_ slow (see the nested for loops below) but nothing
* in here should be really timing critical. -- Cort
*/
static ssize_t ppc_htab_read(struct file * file, char * buf,
static ssize_t ppc_htab_read(struct file * file, char __user * buf,
size_t count, loff_t *ppos)
{
unsigned long mmcr0 = 0, pmc1 = 0, pmc2 = 0;
......@@ -211,13 +211,19 @@ static ssize_t ppc_htab_read(struct file * file, char * buf,
/*
* Allow user to define performance counters and resize the hash table
*/
static ssize_t ppc_htab_write(struct file * file, const char * buffer,
static ssize_t ppc_htab_write(struct file * file, const char __user * ubuffer,
size_t count, loff_t *ppos)
{
#ifdef CONFIG_PPC_STD_MMU
unsigned long tmp;
char buffer[16];
if ( current->uid != 0 )
return -EACCES;
if (strncpy_from_user(buffer, ubuffer, 15))
return -EFAULT;
buffer[15] = 0;
/* don't set the htab size for now */
if ( !strncmp( buffer, "size ", 5) )
return -EBUSY;
......@@ -387,9 +393,10 @@ ppc_htab_lseek(struct file * file, loff_t offset, int orig)
}
int proc_dol2crvec(ctl_table *table, int write, struct file *filp,
void *buffer, size_t *lenp)
void __user *buffer_arg, size_t *lenp)
{
int vleft, first=1, len, left, val;
char __user *buffer = (char __user *) buffer_arg;
#define TMPBUFLEN 256
char buf[TMPBUFLEN], *p;
static const char *sizestrings[4] = {
......@@ -422,12 +429,12 @@ int proc_dol2crvec(ctl_table *table, int write, struct file *filp,
if (write) {
while (left) {
char c;
if(get_user(c,(char *) buffer))
if(get_user(c, buffer))
return -EFAULT;
if (!isspace(c))
break;
left--;
((char *) buffer)++;
buffer++;
}
if (!left)
break;
......@@ -474,7 +481,7 @@ int proc_dol2crvec(ctl_table *table, int write, struct file *filp,
len = strlen(buf);
if (len > left)
len = left;
if(copy_to_user(buffer, buf, len))
if (copy_to_user(buffer, buf, len))
return -EFAULT;
left -= len;
buffer += len;
......
......@@ -441,8 +441,9 @@ int get_fpexc_mode(struct task_struct *tsk, unsigned long adr)
return put_user(val, (unsigned int *) adr);
}
int sys_clone(unsigned long clone_flags, unsigned long usp, int *parent_tidp,
void *child_threadptr, int *child_tidp, int p6,
int sys_clone(unsigned long clone_flags, unsigned long usp,
int __user *parent_tidp, void __user *child_threadptr,
int __user *child_tidp, int p6,
struct pt_regs *regs)
{
CHECK_FULL_REGS(regs);
......@@ -474,7 +475,7 @@ int sys_execve(unsigned long a0, unsigned long a1, unsigned long a2,
int error;
char * filename;
filename = getname((char *) a0);
filename = getname((char __user *) a0);
error = PTR_ERR(filename);
if (IS_ERR(filename))
goto out;
......@@ -484,7 +485,8 @@ int sys_execve(unsigned long a0, unsigned long a1, unsigned long a2,
if (regs->msr & MSR_VEC)
giveup_altivec(current);
#endif /* CONFIG_ALTIVEC */
error = do_execve(filename, (char **) a1, (char **) a2, regs);
error = do_execve(filename, (char __user *__user *) a1,
(char __user *__user *) a2, regs);
if (error == 0)
current->ptrace &= ~PT_DTRACE;
putname(filename);
......
......@@ -83,8 +83,8 @@ sys_sigsuspend(old_sigset_t mask, int p2, int p3, int p4, int p6, int p7,
}
int
sys_rt_sigsuspend(sigset_t *unewset, size_t sigsetsize, int p3, int p4, int p6,
int p7, struct pt_regs *regs)
sys_rt_sigsuspend(sigset_t __user *unewset, size_t sigsetsize, int p3, int p4,
int p6, int p7, struct pt_regs *regs)
{
sigset_t saveset, newset;
......@@ -115,15 +115,15 @@ sys_rt_sigsuspend(sigset_t *unewset, size_t sigsetsize, int p3, int p4, int p6,
int
sys_sigaltstack(const stack_t *uss, stack_t *uoss, int r5, int r6,
int r7, int r8, struct pt_regs *regs)
sys_sigaltstack(const stack_t __user *uss, stack_t __user *uoss, int r5,
int r6, int r7, int r8, struct pt_regs *regs)
{
return do_sigaltstack(uss, uoss, regs->gpr[1]);
}
int
sys_sigaction(int sig, const struct old_sigaction *act,
struct old_sigaction *oact)
sys_sigaction(int sig, const struct old_sigaction __user *act,
struct old_sigaction __user *oact)
{
struct k_sigaction new_ka, old_ka;
int ret;
......@@ -195,14 +195,14 @@ struct rt_sigframe
int sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
struct pt_regs *regs)
{
struct rt_sigframe *rt_sf;
struct rt_sigframe __user *rt_sf;
struct sigcontext sigctx;
struct sigregs *sr;
struct sigregs __user *sr;
elf_gregset_t saved_regs; /* an array of ELF_NGREG unsigned longs */
sigset_t set;
stack_t st;
rt_sf = (struct rt_sigframe *)(regs->gpr[1] + __SIGNAL_FRAMESIZE);
rt_sf = (struct rt_sigframe __user *)(regs->gpr[1] + __SIGNAL_FRAMESIZE);
if (copy_from_user(&sigctx, &rt_sf->uc.uc_mcontext, sizeof(sigctx))
|| copy_from_user(&set, &rt_sf->uc.uc_sigmask, sizeof(set))
|| copy_from_user(&st, &rt_sf->uc.uc_stack, sizeof(st)))
......@@ -220,7 +220,7 @@ int sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
* preamble frame (where registers are stored)
* see handle_signal()
*/
sr = (struct sigregs *) sigctx.regs;
sr = (struct sigregs __user *) sigctx.regs;
if (copy_from_user(saved_regs, &sr->gp_regs, sizeof(sr->gp_regs)))
goto badframe;
saved_regs[PT_MSR] = (regs->msr & ~MSR_USERCHANGE)
......@@ -229,9 +229,6 @@ int sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
if (copy_from_user(current->thread.fpr, &sr->fp_regs,
sizeof(sr->fp_regs)))
goto badframe;
/* This function sets back the stack flags into
the current task structure. */
sys_sigaltstack(&st, NULL, 0, 0, 0, 0, regs);
sigreturn_exit(regs); /* doesn't return here */
return 0;
......@@ -241,10 +238,10 @@ int sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
}
static void
setup_rt_frame(struct pt_regs *regs, struct sigregs *frame,
setup_rt_frame(struct pt_regs *regs, struct sigregs __user *frame,
signed long newsp)
{
struct rt_sigframe *rt_sf = (struct rt_sigframe *) newsp;
struct rt_sigframe __user *rt_sf = (struct rt_sigframe __user *) newsp;
/* Set up preamble frame */
if (verify_area(VERIFY_WRITE, frame, sizeof(*frame)))
......@@ -270,11 +267,11 @@ setup_rt_frame(struct pt_regs *regs, struct sigregs *frame,
set up registers for signal handler
*/
newsp -= __SIGNAL_FRAMESIZE;
if (put_user(regs->gpr[1], (unsigned long *)newsp)
if (put_user(regs->gpr[1], (unsigned long __user *)newsp)
|| get_user(regs->nip, &rt_sf->uc.uc_mcontext.handler)
|| get_user(regs->gpr[3], &rt_sf->uc.uc_mcontext.signal)
|| get_user(regs->gpr[4], (unsigned long *)&rt_sf->pinfo)
|| get_user(regs->gpr[5], (unsigned long *)&rt_sf->puc))
|| get_user(regs->gpr[4], (unsigned long __user *)&rt_sf->pinfo)
|| get_user(regs->gpr[5], (unsigned long __user *)&rt_sf->puc))
goto badframe;
regs->gpr[1] = newsp;
......@@ -297,12 +294,13 @@ setup_rt_frame(struct pt_regs *regs, struct sigregs *frame,
int sys_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
struct pt_regs *regs)
{
struct sigcontext *sc, sigctx;
struct sigregs *sr;
struct sigcontext __user *sc;
struct sigcontext sigctx;
struct sigregs __user *sr;
elf_gregset_t saved_regs; /* an array of ELF_NGREG unsigned longs */
sigset_t set;
sc = (struct sigcontext *)(regs->gpr[1] + __SIGNAL_FRAMESIZE);
sc = (struct sigcontext __user *)(regs->gpr[1] + __SIGNAL_FRAMESIZE);
if (copy_from_user(&sigctx, sc, sizeof(sigctx)))
goto badframe;
......@@ -319,7 +317,7 @@ int sys_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
giveup_fpu(current);
/* restore registers */
sr = (struct sigregs *) sigctx.regs;
sr = (struct sigregs __user *) sigctx.regs;
if (copy_from_user(saved_regs, &sr->gp_regs, sizeof(sr->gp_regs)))
goto badframe;
saved_regs[PT_MSR] = (regs->msr & ~MSR_USERCHANGE)
......@@ -341,10 +339,10 @@ int sys_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
* Set up a signal frame.
*/
static void
setup_frame(struct pt_regs *regs, struct sigregs *frame,
setup_frame(struct pt_regs *regs, struct sigregs __user *frame,
unsigned long newsp)
{
struct sigcontext *sc = (struct sigcontext *) newsp;
struct sigcontext __user *sc = (struct sigcontext __user *) newsp;
if (verify_area(VERIFY_WRITE, frame, sizeof(*frame)))
goto badframe;
......@@ -362,7 +360,7 @@ setup_frame(struct pt_regs *regs, struct sigregs *frame,
current->thread.fpscr = 0; /* turn off all fp exceptions */
newsp -= __SIGNAL_FRAMESIZE;
if (put_user(regs->gpr[1], (unsigned long *)newsp)
if (put_user(regs->gpr[1], (unsigned long __user *)newsp)
|| get_user(regs->nip, &sc->handler)
|| get_user(regs->gpr[3], &sc->signal))
goto badframe;
......@@ -387,8 +385,8 @@ static void
handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
struct pt_regs * regs, unsigned long *newspp, unsigned long frame)
{
struct sigcontext *sc;
struct rt_sigframe *rt_sf;
struct sigcontext __user *sc;
struct rt_sigframe __user *rt_sf;
struct k_sigaction *ka = &current->sighand->action[sig-1];
if (TRAP(regs) == 0x0C00 /* System Call! */
......@@ -408,7 +406,7 @@ handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
if (ka->sa.sa_flags & SA_SIGINFO) {
/* Put a Real Time Context onto stack */
*newspp -= sizeof(*rt_sf);
rt_sf = (struct rt_sigframe *) *newspp;
rt_sf = (struct rt_sigframe __user *) *newspp;
if (verify_area(VERIFY_WRITE, rt_sf, sizeof(*rt_sf)))
goto badframe;
......@@ -432,7 +430,7 @@ handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
} else {
/* Put a sigcontext on the stack */
*newspp -= sizeof(*sc);
sc = (struct sigcontext *) *newspp;
sc = (struct sigcontext __user *) *newspp;
if (verify_area(VERIFY_WRITE, sc, sizeof(*sc)))
goto badframe;
......@@ -516,9 +514,9 @@ int do_signal(sigset_t *oldset, struct pt_regs *regs)
return 0; /* no signals delivered */
if (ka->sa.sa_flags & SA_SIGINFO)
setup_rt_frame(regs, (struct sigregs *) frame, newsp);
setup_rt_frame(regs, (struct sigregs __user *) frame, newsp);
else
setup_frame(regs, (struct sigregs *) frame, newsp);
setup_frame(regs, (struct sigregs __user *) frame, newsp);
return 1;
}
......@@ -52,7 +52,7 @@ check_bugs(void)
* This is really horribly ugly.
*/
int
sys_ipc (uint call, int first, int second, int third, void *ptr, long fifth)
sys_ipc (uint call, int first, int second, int third, void __user *ptr, long fifth)
{
int version, ret;
......@@ -62,7 +62,7 @@ sys_ipc (uint call, int first, int second, int third, void *ptr, long fifth)
ret = -EINVAL;
switch (call) {
case SEMOP:
ret = sys_semop (first, (struct sembuf *)ptr, second);
ret = sys_semop (first, (struct sembuf __user *)ptr, second);
break;
case SEMGET:
ret = sys_semget (first, second, third);
......@@ -73,13 +73,13 @@ sys_ipc (uint call, int first, int second, int third, void *ptr, long fifth)
if (!ptr)
break;
if ((ret = verify_area (VERIFY_READ, ptr, sizeof(long)))
|| (ret = get_user(fourth.__pad, (void **)ptr)))
|| (ret = get_user(fourth.__pad, (void *__user *)ptr)))
break;
ret = sys_semctl (first, second, third, fourth);
break;
}
case MSGSND:
ret = sys_msgsnd (first, (struct msgbuf *) ptr, second, third);
ret = sys_msgsnd (first, (struct msgbuf __user *) ptr, second, third);
break;
case MSGRCV:
switch (version) {
......@@ -90,15 +90,15 @@ sys_ipc (uint call, int first, int second, int third, void *ptr, long fifth)
break;
if ((ret = verify_area (VERIFY_READ, ptr, sizeof(tmp)))
|| (ret = copy_from_user(&tmp,
(struct ipc_kludge *) ptr,
sizeof (tmp)) ? -EFAULT : 0))
(struct ipc_kludge __user *) ptr,
sizeof (tmp)) ? -EFAULT : 0))
break;
ret = sys_msgrcv (first, tmp.msgp, second, tmp.msgtyp,
third);
break;
}
default:
ret = sys_msgrcv (first, (struct msgbuf *) ptr,
ret = sys_msgrcv (first, (struct msgbuf __user *) ptr,
second, fifth, third);
break;
}
......@@ -107,38 +107,28 @@ sys_ipc (uint call, int first, int second, int third, void *ptr, long fifth)
ret = sys_msgget ((key_t) first, second);
break;
case MSGCTL:
ret = sys_msgctl (first, second, (struct msqid_ds *) ptr);
ret = sys_msgctl (first, second, (struct msqid_ds __user *) ptr);
break;
case SHMAT:
switch (version) {
default: {
ulong raddr;
case SHMAT: {
ulong raddr;
if ((ret = verify_area(VERIFY_WRITE, (ulong*) third,
sizeof(ulong))))
break;
ret = sys_shmat (first, (char *) ptr, second, &raddr);
if (ret)
break;
ret = put_user (raddr, (ulong *) third);
if ((ret = verify_area(VERIFY_WRITE, (ulong __user *) third,
sizeof(ulong))))
break;
}
case 1: /* iBCS2 emulator entry point */
if (!segment_eq(get_fs(), get_ds()))
break;
ret = sys_shmat (first, (char *) ptr, second,
(ulong *) third);
ret = sys_shmat (first, (char __user *) ptr, second, &raddr);
if (ret)
break;
}
ret = put_user (raddr, (ulong __user *) third);
break;
}
case SHMDT:
ret = sys_shmdt ((char *)ptr);
ret = sys_shmdt ((char __user *)ptr);
break;
case SHMGET:
ret = sys_shmget (first, second, third);
break;
case SHMCTL:
ret = sys_shmctl (first, second, (struct shmid_ds *) ptr);
ret = sys_shmctl (first, second, (struct shmid_ds __user *) ptr);
break;
}
......@@ -149,7 +139,7 @@ sys_ipc (uint call, int first, int second, int third, void *ptr, long fifth)
* sys_pipe() is the normal C calling standard for creating
* a pipe. It's not the way unix traditionally does this, though.
*/
int sys_pipe(int *fildes)
int sys_pipe(int __user *fildes)
{
int fd[2];
int error;
......@@ -219,7 +209,7 @@ ppc_select(int n, fd_set *inp, fd_set *outp, fd_set *exp, struct timeval *tvp)
{
if ( (unsigned long)n >= 4096 )
{
unsigned long *buffer = (unsigned long *)n;
unsigned long __user *buffer = (unsigned long __user *)n;
if (verify_area(VERIFY_READ, buffer, 5*sizeof(unsigned long))
|| __get_user(n, buffer)
|| __get_user(inp, ((fd_set **)(buffer+1)))
......@@ -231,7 +221,7 @@ ppc_select(int n, fd_set *inp, fd_set *outp, fd_set *exp, struct timeval *tvp)
return sys_select(n, inp, outp, exp, tvp);
}
int sys_uname(struct old_utsname * name)
int sys_uname(struct old_utsname __user * name)
{
int err = -EFAULT;
......@@ -242,7 +232,7 @@ int sys_uname(struct old_utsname * name)
return err;
}
int sys_olduname(struct oldold_utsname * name)
int sys_olduname(struct oldold_utsname __user * name)
{
int error;
......
......@@ -281,7 +281,7 @@ emulate_instruction(struct pt_regs *regs)
return retval;
CHECK_FULL_REGS(regs);
if (get_user(instword, (uint *)(regs->nip)))
if (get_user(instword, (uint __user *)(regs->nip)))
return -EFAULT;
/* Emulate the mfspr rD, PVR.
......
......@@ -7,7 +7,7 @@
* See arch/ppc/kernel/syscalls.c for ugly details..
*/
struct ipc_kludge {
struct msgbuf *msgp;
struct msgbuf __user *msgp;
long msgtyp;
};
......
......@@ -32,7 +32,7 @@
#define __access_ok(addr,size) (__kernel_ok || __user_ok((addr),(size)))
#define access_ok(type,addr,size) __access_ok((unsigned long)(addr),(size))
extern inline int verify_area(int type, const void * addr, unsigned long size)
extern inline int verify_area(int type, const void __user * addr, unsigned long size)
{
return access_ok(type,addr,size) ? 0 : -EFAULT;
}
......@@ -225,45 +225,46 @@ do { \
/* more complex routines */
extern int __copy_tofrom_user(void *to, const void *from, unsigned long size);
extern int __copy_tofrom_user(void __user *to, const void __user *from,
unsigned long size);
extern inline unsigned long
copy_from_user(void *to, const void *from, unsigned long n)
copy_from_user(void *to, const void __user *from, unsigned long n)
{
unsigned long over;
if (access_ok(VERIFY_READ, from, n))
return __copy_tofrom_user(to, from, n);
return __copy_tofrom_user((void __user *)to, from, n);
if ((unsigned long)from < TASK_SIZE) {
over = (unsigned long)from + n - TASK_SIZE;
return __copy_tofrom_user(to, from, n - over) + over;
return __copy_tofrom_user((void __user *)to, from, n - over) + over;
}
return n;
}
extern inline unsigned long
copy_to_user(void *to, const void *from, unsigned long n)
copy_to_user(void __user *to, const void *from, unsigned long n)
{
unsigned long over;
if (access_ok(VERIFY_WRITE, to, n))
return __copy_tofrom_user(to, from, n);
return __copy_tofrom_user(to, (void __user *) from, n);
if ((unsigned long)to < TASK_SIZE) {
over = (unsigned long)to + n - TASK_SIZE;
return __copy_tofrom_user(to, from, n - over) + over;
return __copy_tofrom_user(to, (void __user *) from, n - over) + over;
}
return n;
}
#define __copy_from_user(to, from, size) \
__copy_tofrom_user((to), (from), (size))
__copy_tofrom_user((void __user *)(to), (from), (size))
#define __copy_to_user(to, from, size) \
__copy_tofrom_user((to), (from), (size))
__copy_tofrom_user((to), (void __user *)(from), (size))
extern unsigned long __clear_user(void *addr, unsigned long size);
extern unsigned long __clear_user(void __user *addr, unsigned long size);
extern inline unsigned long
clear_user(void *addr, unsigned long size)
clear_user(void __user *addr, unsigned long size)
{
if (access_ok(VERIFY_WRITE, addr, size))
return __clear_user(addr, size);
......@@ -274,10 +275,10 @@ clear_user(void *addr, unsigned long size)
return size;
}
extern int __strncpy_from_user(char *dst, const char *src, long count);
extern int __strncpy_from_user(char *dst, const char __user *src, long count);
extern inline long
strncpy_from_user(char *dst, const char *src, long count)
strncpy_from_user(char *dst, const char __user *src, long count)
{
if (access_ok(VERIFY_READ, src, 1))
return __strncpy_from_user(dst, src, count);
......@@ -290,7 +291,7 @@ strncpy_from_user(char *dst, const char *src, long count)
* Return 0 for error
*/
extern int __strnlen_user(const char *str, long len, unsigned long top);
extern int __strnlen_user(const char __user *str, long len, unsigned long top);
/*
* Returns the length of the string at str (including the null byte),
......@@ -300,7 +301,7 @@ extern int __strnlen_user(const char *str, long len, unsigned long top);
* The `top' parameter to __strnlen_user is to make sure that
* we can never overflow from the user area into kernel space.
*/
extern __inline__ int strnlen_user(const char *str, long len)
extern __inline__ int strnlen_user(const char __user *str, long len)
{
unsigned long top = __kernel_ok? ~0UL: TASK_SIZE - 1;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment