tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 4b35035bcf80ddb47c0112c4fbd84a63a2836a18
commit: 5bd009c7c9a9e888077c07535dc0c70aeab242c3 mm: madvise: return correct bytes advised with process_madvise
date: 3 months ago
compiler: mips-linux-gcc (GCC) 11.3.0
reproduce (cppcheck warning):
# apt-get install cppcheck
git checkout 5bd009c7c9a9e888077c07535dc0c70aeab242c3
cppcheck --quiet --enable=style,performance,portability --template=gcc FILE
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
cppcheck warnings: (new ones prefixed by >>)
>> mm/madvise.c:1438:6: warning: Redundant assignment of 'ret' to itself. [selfAssignment]
ret = (total_len - iov_iter_count(&iter)) ? : ret;
^
cppcheck possible warnings: (new ones prefixed by >>, may not real problems)
mm/madvise.c:123:28: warning: Parameter 'anon_name' can be declared with const [constParameter]
struct anon_vma_name *anon_name)
^
vim +/ret +1438 mm/madvise.c
1378
1379 SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
1380 size_t, vlen, int, behavior, unsigned int, flags)
1381 {
1382 ssize_t ret;
1383 struct iovec iovstack[UIO_FASTIOV], iovec;
1384 struct iovec *iov = iovstack;
1385 struct iov_iter iter;
1386 struct task_struct *task;
1387 struct mm_struct *mm;
1388 size_t total_len;
1389 unsigned int f_flags;
1390
1391 if (flags != 0) {
1392 ret = -EINVAL;
1393 goto out;
1394 }
1395
1396 ret = import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
1397 if (ret < 0)
1398 goto out;
1399
1400 task = pidfd_get_task(pidfd, &f_flags);
1401 if (IS_ERR(task)) {
1402 ret = PTR_ERR(task);
1403 goto free_iov;
1404 }
1405
1406 if (!process_madvise_behavior_valid(behavior)) {
1407 ret = -EINVAL;
1408 goto release_task;
1409 }
1410
1411 /* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */
1412 mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
1413 if (IS_ERR_OR_NULL(mm)) {
1414 ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
1415 goto release_task;
1416 }
1417
1418 /*
1419 * Require CAP_SYS_NICE for influencing process performance. Note that
1420 * only non-destructive hints are currently supported.
1421 */
1422 if (!capable(CAP_SYS_NICE)) {
1423 ret = -EPERM;
1424 goto release_mm;
1425 }
1426
1427 total_len = iov_iter_count(&iter);
1428
1429 while (iov_iter_count(&iter)) {
1430 iovec = iov_iter_iovec(&iter);
1431 ret = do_madvise(mm, (unsigned long)iovec.iov_base,
1432 iovec.iov_len, behavior);
1433 if (ret < 0)
1434 break;
1435 iov_iter_advance(&iter, iovec.iov_len);
1436 }
1437
> 1438 ret = (total_len - iov_iter_count(&iter)) ? : ret;
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hello Andrew,
On 6/18/2022 4:34 AM, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 4b35035bcf80ddb47c0112c4fbd84a63a2836a18
> commit: 5bd009c7c9a9e888077c07535dc0c70aeab242c3 mm: madvise: return correct bytes advised with process_madvise
> date: 3 months ago
> compiler: mips-linux-gcc (GCC) 11.3.0
> reproduce (cppcheck warning):
> # apt-get install cppcheck
> git checkout 5bd009c7c9a9e888077c07535dc0c70aeab242c3
> cppcheck --quiet --enable=style,performance,portability --template=gcc FILE
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[email protected]>
>
>
> cppcheck warnings: (new ones prefixed by >>)
>>> mm/madvise.c:1438:6: warning: Redundant assignment of 'ret' to itself. [selfAssignment]
> ret = (total_len - iov_iter_count(&iter)) ? : ret;
Other way to avoid this warning is by creating another local variable
that holds the total bytes processed. Having another local variable to
get rid off some compilation warning doesn't seem proper to me. So,
leaving this warning unless you ask me to fix this.
Thanks,
Charan
On Sat 18-06-22 11:25:43, Charan Teja Kalla wrote:
> Hello Andrew,
>
> On 6/18/2022 4:34 AM, kernel test robot wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head: 4b35035bcf80ddb47c0112c4fbd84a63a2836a18
> > commit: 5bd009c7c9a9e888077c07535dc0c70aeab242c3 mm: madvise: return correct bytes advised with process_madvise
> > date: 3 months ago
> > compiler: mips-linux-gcc (GCC) 11.3.0
> > reproduce (cppcheck warning):
> > # apt-get install cppcheck
> > git checkout 5bd009c7c9a9e888077c07535dc0c70aeab242c3
> > cppcheck --quiet --enable=style,performance,portability --template=gcc FILE
> >
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <[email protected]>
> >
> >
> > cppcheck warnings: (new ones prefixed by >>)
> >>> mm/madvise.c:1438:6: warning: Redundant assignment of 'ret' to itself. [selfAssignment]
> > ret = (total_len - iov_iter_count(&iter)) ? : ret;
>
> Other way to avoid this warning is by creating another local variable
> that holds the total bytes processed. Having another local variable to
> get rid off some compilation warning doesn't seem proper to me. So,
> leaving this warning unless you ask me to fix this.
Is this a new warning? I do not see it supported by my gcc 10.x. Do we
plan to have it enabled by default? I do not see anything wrong with the
above code and I think this is not an unusual pattern in the kernel.
While you could go with
if (rotal_len - iov_iter_count(&iter))
ret = rotal_len - iov_iter_count(&iter);
or do the same with a temporary variable but I am not really sure this would
add to the readability much.
--
Michal Hocko
SUSE Labs
On Mon, 20 Jun 2022, Michal Hocko wrote:
> On Sat 18-06-22 11:25:43, Charan Teja Kalla wrote:
> > Hello Andrew,
> >
> > On 6/18/2022 4:34 AM, kernel test robot wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head: 4b35035bcf80ddb47c0112c4fbd84a63a2836a18
> > > commit: 5bd009c7c9a9e888077c07535dc0c70aeab242c3 mm: madvise: return correct bytes advised with process_madvise
> > > date: 3 months ago
> > > compiler: mips-linux-gcc (GCC) 11.3.0
> > > reproduce (cppcheck warning):
> > > # apt-get install cppcheck
> > > git checkout 5bd009c7c9a9e888077c07535dc0c70aeab242c3
> > > cppcheck --quiet --enable=style,performance,portability --template=gcc FILE
> > >
> > > If you fix the issue, kindly add following tag where applicable
> > > Reported-by: kernel test robot <[email protected]>
> > >
> > >
> > > cppcheck warnings: (new ones prefixed by >>)
> > >>> mm/madvise.c:1438:6: warning: Redundant assignment of 'ret' to itself. [selfAssignment]
> > > ret = (total_len - iov_iter_count(&iter)) ? : ret;
> >
> > Other way to avoid this warning is by creating another local variable
> > that holds the total bytes processed. Having another local variable to
> > get rid off some compilation warning doesn't seem proper to me. So,
> > leaving this warning unless you ask me to fix this.
>
> Is this a new warning? I do not see it supported by my gcc 10.x. Do we
cppcheck is a static analysis tool. It looks like it doesn't have a
proper understanding of ?:
julia
> plan to have it enabled by default? I do not see anything wrong with the
> above code and I think this is not an unusual pattern in the kernel.
> While you could go with
> if (rotal_len - iov_iter_count(&iter))
> ret = rotal_len - iov_iter_count(&iter);
>
> or do the same with a temporary variable but I am not really sure this would
> add to the readability much.
> --
> Michal Hocko
> SUSE Labs
> _______________________________________________
> kbuild-all mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
>
On Mon 20-06-22 12:54:56, Julia Lawall wrote:
>
>
> On Mon, 20 Jun 2022, Michal Hocko wrote:
>
> > On Sat 18-06-22 11:25:43, Charan Teja Kalla wrote:
> > > Hello Andrew,
> > >
> > > On 6/18/2022 4:34 AM, kernel test robot wrote:
> > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > head: 4b35035bcf80ddb47c0112c4fbd84a63a2836a18
> > > > commit: 5bd009c7c9a9e888077c07535dc0c70aeab242c3 mm: madvise: return correct bytes advised with process_madvise
> > > > date: 3 months ago
> > > > compiler: mips-linux-gcc (GCC) 11.3.0
> > > > reproduce (cppcheck warning):
> > > > # apt-get install cppcheck
> > > > git checkout 5bd009c7c9a9e888077c07535dc0c70aeab242c3
> > > > cppcheck --quiet --enable=style,performance,portability --template=gcc FILE
> > > >
> > > > If you fix the issue, kindly add following tag where applicable
> > > > Reported-by: kernel test robot <[email protected]>
> > > >
> > > >
> > > > cppcheck warnings: (new ones prefixed by >>)
> > > >>> mm/madvise.c:1438:6: warning: Redundant assignment of 'ret' to itself. [selfAssignment]
> > > > ret = (total_len - iov_iter_count(&iter)) ? : ret;
> > >
> > > Other way to avoid this warning is by creating another local variable
> > > that holds the total bytes processed. Having another local variable to
> > > get rid off some compilation warning doesn't seem proper to me. So,
> > > leaving this warning unless you ask me to fix this.
> >
> > Is this a new warning? I do not see it supported by my gcc 10.x. Do we
>
> cppcheck is a static analysis tool. It looks like it doesn't have a
> proper understanding of ?:
Ohh, thanks for the clarification! I thought this was a gcc feature.
Then I would suggest to report a bug report against the static checker
rather than making any changes to the kernel to workaround it.
--
Michal Hocko
SUSE Labs
On 6/21/2022 3:14 AM, Michal Hocko wrote:
> On Mon 20-06-22 12:54:56, Julia Lawall wrote:
>>
>>
>> On Mon, 20 Jun 2022, Michal Hocko wrote:
>>
>>> On Sat 18-06-22 11:25:43, Charan Teja Kalla wrote:
>>>> Hello Andrew,
>>>>
[...]
>>>>> cppcheck warnings: (new ones prefixed by >>)
>>>>>>> mm/madvise.c:1438:6: warning: Redundant assignment of 'ret' to itself. [selfAssignment]
>>>>> ret = (total_len - iov_iter_count(&iter)) ? : ret;
>>>>
>>>> Other way to avoid this warning is by creating another local variable
>>>> that holds the total bytes processed. Having another local variable to
>>>> get rid off some compilation warning doesn't seem proper to me. So,
>>>> leaving this warning unless you ask me to fix this.
>>>
>>> Is this a new warning? I do not see it supported by my gcc 10.x. Do we
>>
>> cppcheck is a static analysis tool. It looks like it doesn't have a
>> proper understanding of ?:
>
> Ohh, thanks for the clarification! I thought this was a gcc feature.
> Then I would suggest to report a bug report against the static checker
> rather than making any changes to the kernel to workaround it.
>
Hi all,
Sorry for the inconvenience, we have added the warning to ignore list
to avoid reporting it again.
Best Regards,
Rong Chen