I was cc'd on a patch where structs were used on stack instead
of using pointers to the structs. This can cause defects when
the calling function modifies the stack struct instead of the
calling function's struct.
Possible patch below, but it may be overkill for the number of
instances
where this is actually an issue.
Thoughts?
There are what seems to be some false positives for a few of the
.h files in include/linux/... where the false positives are for
very small structs where the indirection via a pointer might be
slower than than the stack use.
For instance: (some duplicates removed)
$ git ls-files -- "include/linux/*.h" | \
while read file ; do ./scripts/checkpatch.pl -f --types=aggregate_on_stack $file --no-summary --quiet; done
WARNING: Unusual use of 'struct bvec_iter' on stack
#520: FILE: include/linux/bio.h:520:
+void zero_fill_bio_iter(struct bio *bio, struct bvec_iter iter);
WARNING: Unusual use of 'struct fd' on stack
#433: FILE: include/linux/bpf.h:433:
+struct bpf_map *__bpf_map_get(struct fd f);
WARNING: Unusual use of 'struct ceph_vino' on stack
#465: FILE: include/linux/ceph/osd_client.h:465:
+extern struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *,
+ struct ceph_file_layout *layout,
+ struct ceph_vino vino,
+ u64 offset, u64 *len,
+ unsigned int which, int num_ops,
+ int opcode, int flags,
+ struct ceph_snap_context *snapc,
+ u32 truncate_seq, u64 truncate_size,
+ bool use_mempool);
[]
WARNING: Unusual use of 'union extcon_property_value' on stack
#63: FILE: include/linux/extcon-provider.h:63:
+extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
+ unsigned int prop,
+ union extcon_property_value prop_val);
[]
WARNING: Unusual use of 'struct ppa_addr' on stack
#528: FILE: include/linux/lightnvm.h:528:
+extern int nvm_get_chunk_meta(struct nvm_tgt_dev *tgt_dev,
+ struct nvm_chk_meta *meta, struct ppa_addr ppa,
+ int nchks);
[]
WARNING: Unusual use of 'struct pin_cookie' on stack
#367: FILE: include/linux/lockdep.h:367:
+extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
[]
WARNING: Unusual use of 'struct kqid' on stack
#77: FILE: include/linux/quota.h:77:
+extern bool qid_eq(struct kqid left, struct kqid right);
[]
WARNING: Unusual use of 'struct reg_field' on stack
#1051: FILE: include/linux/regmap.h:1051:
+struct regmap_field *devm_regmap_field_alloc(struct device *dev,
+ struct regmap *regmap, struct reg_field reg_field);
WARNING: Unusual use of 'struct rpmsg_channel_info' on stack
#121: FILE: include/linux/rpmsg.h:121:
+struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
+ rpmsg_rx_cb_t cb, void *priv,
+ struct rpmsg_channel_info chinfo);
WARNING: Unusual use of 'struct rtc_time' on stack
#26: FILE: include/linux/rtc.h:26:
+ktime_t rtc_tm_to_ktime(struct rtc_time tm);
WARNING: Unusual use of 'struct timespec64' on stack
#193: FILE: include/linux/rtc.h:193:
+extern int rtc_set_ntp_time(struct timespec64 now, unsigned long *target_nsec);
[]
WARNING: Unusual use of 'struct tnum' on stack
#25: FILE: include/linux/tnum.h:25:
+struct tnum tnum_lshift(struct tnum a, u8 shift);
[]
WARNING: Unusual use of 'struct vring' on stack
#81: FILE: include/linux/virtio_ring.h:81:
+struct virtqueue *__vring_new_virtqueue(unsigned int index,
+ struct vring vring,
+ struct virtio_device *vdev,
+ bool weak_barriers,
+ bool ctx,
+ bool (*notify)(struct virtqueue *),
+ void (*callback)(struct virtqueue *),
+ const char *name);
WARNING: Unusual use of 'struct vmci_handle' on stack
#39: FILE: include/linux/vmw_vmci_api.h:39:
+int vmci_datagram_destroy_handle(struct vmci_handle handle);
---
scripts/checkpatch.pl | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34e4683de7a3..6ec86d8a4983 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6076,8 +6076,10 @@ sub process {
# check for function definitions
if ($perl_version_ok &&
defined $stat &&
- $stat =~ /^.\s*(?:$Storage\s+)?$Type\s*($Ident)\s*$balanced_parens\s*{/s) {
+ $stat =~ /^.\s*(?:$Storage\s+)?$Type\s*($Ident)\s*$balanced_parens\s*([\{;])/s) {
$context_function = $1;
+ my $args = $2;
+ my $term = $3;
# check for multiline function definition with misplaced open brace
my $ok = 0;
@@ -6088,12 +6090,25 @@ sub process {
$herectx .= $rl . "\n";
$ok = 1 if ($rl =~ /^[ \+]\{/);
$ok = 1 if ($rl =~ /\{/ && $n == 0);
- last if $rl =~ /^[ \+].*\{/;
+ last if ($rl =~ /^[ \+].*[\{;]/);
}
- if (!$ok) {
+ if (!$ok || $term eq '{') {
ERROR("OPEN_BRACE",
"open brace '{' following function definitions go on the next line\n" . $herectx);
}
+
+# check for struct or union uses on stack that might use struct or union *
+
+ while ($args =~ /(?:$Storage\s+)?($Type)\s*($Ident(?:\s*\[\s*\])?)?\s*,?/g) {
+ my $type = trim($1);
+ my $ident = defined($2) ? trim($2) : "";
+ if ($type =~ /(?:union|struct)/ &&
+ !($type =~ /(?:\*|\bconst|\])$/ ||
+ $ident =~ /\]$/)) {
+ WARN("AGGREGATE_ON_STACK",
+ "Unusual use of '$type' on stack\n" . $herectx);
+ }
+ }
}
# checks for new __setup's
On Thu, 26 Jul 2018 11:27:50 -0700 Joe Perches <[email protected]> wrote:
> I was cc'd on a patch where structs were used on stack instead
> of using pointers to the structs.
"passed by value" is a good term for this practice.
This can cause defects when
> the calling function modifies the stack struct instead of the
"called"
> calling function's struct.
>
> Possible patch below, but it may be overkill for the number of
> instances
> where this is actually an issue.
>
> Thoughts?
Seems worthwhile.
> There are what seems to be some false positives for a few of the
> .h files in include/linux/... where the false positives are for
> very small structs where the indirection via a pointer might be
> slower than than the stack use.
>
> For instance: (some duplicates removed)
I'll give it a spin, see how noisy it is.
On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
> I'll give it a spin, see how noisy it is.
Actually, I would prefer if the message, changelog and title
used the term "passed by value". It's a more familiar term
and it is possible for a passed-by-value aggregate to in fact
be passed in registers.
On Thu, 2018-07-26 at 12:28 -0700, Andrew Morton wrote:
> On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
>
> > I'll give it a spin, see how noisy it is.
>
> Actually, I would prefer if the message, changelog and title
> used the term "passed by value". It's a more familiar term
> and it is possible for a passed-by-value aggregate to in fact
> be passed in registers.
RFC, No worries, I'll change it if it's OK.
I'm testing it right now against the last 5000 commits
(which takes awhile here) via
$ git log --no-merges --format=oneline -5000 | \
cut -f1 -d" " | \
while read commit ; do \
echo $commit; \
./scripts/checkpatch.pl --git $commit --types=aggregate_on_stack --quiet --no-summary ; \
done
It doesn't seem noisy at all, but maybe there are a few
known structs like "struct timespec64" that could be
excluded.
The only real hits so far are:
commit f2fb56afba11426ee5c9603b28a9827c530909c0
WARNING: Unusual use of 'struct msm_display_topology' on stack
#28374: FILE: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:149:
+enum dpu_rm_topology_name
+dpu_rm_get_topology_name(struct msm_display_topology topology)
+{
WARNING: Unusual use of 'struct msm_display_topology' on stack
#29021: FILE: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:796:
+static int _dpu_rm_populate_requirements(
+ struct dpu_rm *rm,
+ struct drm_encoder *enc,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ struct dpu_rm_requirements *reqs,
+ struct msm_display_topology req_topology)
+{
WARNING: Unusual use of 'struct msm_display_topology' on stack
#29203: FILE: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:978:
+int dpu_rm_reserve(
+ struct dpu_rm *rm,
+ struct drm_encoder *enc,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ struct msm_display_topology topology,
+ bool test_only)
+{
WARNING: Unusual use of 'struct msm_display_topology' on stack
#29443: FILE: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h:133:
+int dpu_rm_reserve(struct dpu_rm *rm,
+ struct drm_encoder *drm_enc,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ struct msm_display_topology topology,
+ bool test_only);
WARNING: Unusual use of 'struct msm_display_topology' on stack
#29506: FILE: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h:196:
+enum dpu_rm_topology_name
+dpu_rm_get_topology_name(struct msm_display_topology topology);
and
33477d84c26bbfa626da2c032e567a90dd70a528
WARNING: Unusual use of 'struct cppc_perf_fb_ctrs' on stack
#45: FILE: drivers/cpufreq/cppc_cpufreq.c:307:
+static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
+ struct cppc_perf_fb_ctrs fb_ctrs_t0,
+ struct cppc_perf_fb_ctrs fb_ctrs_t1)
+{
WARNING: Unusual use of 'struct cppc_perf_fb_ctrs' on stack
#45: FILE: drivers/cpufreq/cppc_cpufreq.c:307:
+static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
+ struct cppc_perf_fb_ctrs fb_ctrs_t0,
+ struct cppc_perf_fb_ctrs fb_ctrs_t1)
+{
On Thu, 26 Jul 2018 13:05:29 -0700 Joe Perches <[email protected]> wrote:
> On Thu, 2018-07-26 at 12:28 -0700, Andrew Morton wrote:
> > On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
> >
> > > I'll give it a spin, see how noisy it is.
> >
> > Actually, I would prefer if the message, changelog and title
> > used the term "passed by value". It's a more familiar term
> > and it is possible for a passed-by-value aggregate to in fact
> > be passed in registers.
>
> RFC, No worries, I'll change it if it's OK.
>
> I'm testing it right now against the last 5000 commits
> (which takes awhile here) via
>
> $ git log --no-merges --format=oneline -5000 | \
> cut -f1 -d" " | \
> while read commit ; do \
> echo $commit; \
> ./scripts/checkpatch.pl --git $commit --types=aggregate_on_stack --quiet --no-summary ; \
> done
>
> It doesn't seem noisy at all, but maybe there are a few
> known structs like "struct timespec64" that could be
> excluded.
>
> The only real hits so far are:
>
> commit f2fb56afba11426ee5c9603b28a9827c530909c0
> WARNING: Unusual use of 'struct msm_display_topology' on stack
> #28374: FILE: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:149:
> +enum dpu_rm_topology_name
> +dpu_rm_get_topology_name(struct msm_display_topology topology)
> +{
hm. 12 bytes. I don't know if this would be more efficient than using
const struct msm_display_topology*.
> and
>
> 33477d84c26bbfa626da2c032e567a90dd70a528
> WARNING: Unusual use of 'struct cppc_perf_fb_ctrs' on stack
> #45: FILE: drivers/cpufreq/cppc_cpufreq.c:307:
> +static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
> + struct cppc_perf_fb_ctrs fb_ctrs_t0,
> + struct cppc_perf_fb_ctrs fb_ctrs_t1)
> +{
Two 32-byte structures? That seems excessive.
Yes, a warning which sends developers back for a bit more thinnking
sounds useful.
From: Andrew Morton
> Sent: 26 July 2018 20:28
> On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
>
> > I'll give it a spin, see how noisy it is.
>
> Actually, I would prefer if the message, changelog and title
> used the term "passed by value". It's a more familiar term
> and it is possible for a passed-by-value aggregate to in fact
> be passed in registers.
You need to detect (and ignore) 'small' structures.
Quite a few ABI pass small structures by value in register(s)
or directly on the stack.
So it can make sense to encapsulate an integer value in a
structure in order to get strong typing.
It would, for instance, make sense to do that for user addresses.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, 2018-07-27 at 10:04 +0000, David Laight wrote:
> From: Andrew Morton
> > Sent: 26 July 2018 20:28
> > On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
> >
> > > I'll give it a spin, see how noisy it is.
> >
> > Actually, I would prefer if the message, changelog and title
> > used the term "passed by value". It's a more familiar term
> > and it is possible for a passed-by-value aggregate to in fact
> > be passed in registers.
>
> You need to detect (and ignore) 'small' structures.
checkpatch is stupid and basically can't do that
as it has no context other than the current line.
It would need a list of specific struct types to
ignore. Care to create and send that list to me?
From an earlier message:
On Thu, 2018-07-26 at 13:05 -0700, Joe Perches wrote:
> It doesn't seem noisy at all, but maybe there are a few
> known structs like "struct timespec64" that could be
> excluded.
From: Joe Perches
> Sent: 27 July 2018 11:09
> On Fri, 2018-07-27 at 10:04 +0000, David Laight wrote:
> > From: Andrew Morton
> > > Sent: 26 July 2018 20:28
> > > On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
> > >
> > > > I'll give it a spin, see how noisy it is.
> > >
> > > Actually, I would prefer if the message, changelog and title
> > > used the term "passed by value". It's a more familiar term
> > > and it is possible for a passed-by-value aggregate to in fact
> > > be passed in registers.
> >
> > You need to detect (and ignore) 'small' structures.
>
> checkpatch is stupid and basically can't do that
> as it has no context other than the current line.
>
> It would need a list of specific struct types to
> ignore. Care to create and send that list to me?
Does it even have the type?
If it has the prototype it could ignore aggregates that
are marked 'const'.
At least we're not in the K&R days where missing out the
& got very confusing.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, 2018-07-27 at 10:21 +0000, David Laight wrote:
> From: Joe Perches Sent: 27 July 2018 11:09
> > On Fri, 2018-07-27 at 10:04 +0000, David Laight wrote:
> > > From: Andrew Morton Sent: 26 July 2018 20:28
> > > > On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
> > > >
> > > > > I'll give it a spin, see how noisy it is.
> > > >
> > > > Actually, I would prefer if the message, changelog and title
> > > > used the term "passed by value". It's a more familiar term
> > > > and it is possible for a passed-by-value aggregate to in fact
> > > > be passed in registers.
> > >
> > > You need to detect (and ignore) 'small' structures.
> >
> > checkpatch is stupid and basically can't do that
> > as it has no context other than the current line.
> >
> > It would need a list of specific struct types to
> > ignore. Care to create and send that list to me?
>
> Does it even have the type?
Yes, kinda. But only on the line being matched.
i.e.: <const> [struct or union] [type] [name]
> If it has the prototype it could ignore aggregates that
> are marked 'const'.
checkpatch has no visibility of any prototype.
It might make sense for this sort of check to be
added to coccinelle or maybe as a compiler warning
when the struct is larger than some size.
Original thread for Julia:
https://lore.kernel.org/patchwork/patch/967890/
On Fri, 27 Jul 2018, Joe Perches wrote:
> On Fri, 2018-07-27 at 10:21 +0000, David Laight wrote:
> > From: Joe Perches Sent: 27 July 2018 11:09
> > > On Fri, 2018-07-27 at 10:04 +0000, David Laight wrote:
> > > > From: Andrew Morton Sent: 26 July 2018 20:28
> > > > > On Thu, 26 Jul 2018 12:25:33 -0700 Andrew Morton <[email protected]> wrote:
> > > > >
> > > > > > I'll give it a spin, see how noisy it is.
> > > > >
> > > > > Actually, I would prefer if the message, changelog and title
> > > > > used the term "passed by value". It's a more familiar term
> > > > > and it is possible for a passed-by-value aggregate to in fact
> > > > > be passed in registers.
> > > >
> > > > You need to detect (and ignore) 'small' structures.
> > >
> > > checkpatch is stupid and basically can't do that
> > > as it has no context other than the current line.
> > >
> > > It would need a list of specific struct types to
> > > ignore. Care to create and send that list to me?
> >
> > Does it even have the type?
>
> Yes, kinda. But only on the line being matched.
>
> i.e.: <const> [struct or union] [type] [name]
>
> > If it has the prototype it could ignore aggregates that
> > are marked 'const'.
>
> checkpatch has no visibility of any prototype.
>
> It might make sense for this sort of check to be
> added to coccinelle or maybe as a compiler warning
> when the struct is larger than some size.
>
> Original thread for Julia:
> https://lore.kernel.org/patchwork/patch/967890/
Coccinelle doesn't directly know the size of the structure, but it can
count the number of fields. Maybe a case with an update in the function
body or at least 3 fields is worth reporting on?
julia
On Sat, 2018-07-28 at 08:25 +0200, Julia Lawall wrote:
> On Fri, 27 Jul 2018, Joe Perches wrote:
[]
> > It might make sense for this sort of check to be
> > added to coccinelle or maybe as a compiler warning
> > when the struct is larger than some size.
> >
> > Original thread for Julia:
> > https://lore.kernel.org/patchwork/patch/967890/
>
> Coccinelle doesn't directly know the size of the structure, but it can
> count the number of fields. Maybe a case with an update in the function
> body
Perhaps this might be the most useful to check.
> or at least 3 fields is worth reporting on?
a struct with 3 chars or bools might be faster
by value, but maybe for structs with arrays or
other structs.
For instance:
lib/vsprintf.c uses struct printf_spec which
is 16 bytes and that fits nicely in a
(unintentionally sent partial reply, better now)
On Sat, 2018-07-28 at 08:25 +0200, Julia Lawall wrote:
> On Fri, 27 Jul 2018, Joe Perches wrote:
[]
> > It might make sense for this sort of check to be
> > added to coccinelle or maybe as a compiler warning
> > when the struct is larger than some size.
> >
> > Original thread for Julia:
> > https://lore.kernel.org/patchwork/patch/967890/
>
> Coccinelle doesn't directly know the size of the structure, but it can
> count the number of fields. Maybe a case with an update in the function
> body
Perhaps this might be the most useful to check.
> or at least 3 fields is worth reporting on?
Maybe, maybe not. For instance:
lib/vsprintf.c uses struct printf_spec which is
5 fields totaling 8 bytes and that fits nicely in
a single register on x86-64 so there are good
reasons why structs could be passed by value.
Maybe structs with arrays or other structs would
make more sense.
On Sat, 28 Jul 2018, Joe Perches wrote:
> On Sat, 2018-07-28 at 08:25 +0200, Julia Lawall wrote:
> > On Fri, 27 Jul 2018, Joe Perches wrote:
> []
> > > It might make sense for this sort of check to be
> > > added to coccinelle or maybe as a compiler warning
> > > when the struct is larger than some size.
> > >
> > > Original thread for Julia:
> > > https://lore.kernel.org/patchwork/patch/967890/
> >
> > Coccinelle doesn't directly know the size of the structure, but it can
> > count the number of fields. Maybe a case with an update in the function
> > body
>
> Perhaps this might be the most useful to check.
>
> > or at least 3 fields is worth reporting on?
>
> a struct with 3 chars or bools might be faster
> by value, but maybe for structs with arrays or
> other structs.
>
> For instance:
>
> lib/vsprintf.c uses struct printf_spec which
> is 16 bytes and that fits nicely in a
This message seems to be cut off. I got around 70 results, of which 23
are from vsprintf. Perhaps the simplest would be to print the structure
declaration with the warning message, so the user could easily check the
results.
julia