* Fix for synchronization issue in IPV6 implementation in smack module(v3.18)
@ 2015-01-08 16:41 Vishal Goel
2015-01-09 0:09 ` Casey Schaufler
2015-01-09 13:42 ` Ahmed S. Darwish
0 siblings, 2 replies; 4+ messages in thread
From: Vishal Goel @ 2015-01-08 16:41 UTC (permalink / raw)
To: linux-kernel, linux-security-module, casey
[PATCH] This patch fixes the synchronization issue in IPv6
implementation. Previously there was no synchronization mechanism used while
accessing(adding/reading/deletion) smk_ipv6_port_list. It could be possible
that when one thread is reading the list, at the same time another thread is
adding/deleting in the list.So it is possible that reader thread will read
the inaccurate or incomplete list. So to make sure that reader thread will
read the accurate list, rcu mechanism has been used while accessing the
list.RCU allows readers to access a data structure even when it is in the
process of being updated
Signed-off-by: Vishal Goel <vishal.goel@samsung.com>
Himanshu Shukla <himanshu.sh@samsung.com>
---
security/smack/smack_lsm.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index d515ec2..b3427ee 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -52,6 +52,7 @@
#define SMK_RECEIVING 1
#define SMK_SENDING 2
+DEFINE_MUTEX(smack_ipv6_lock);
LIST_HEAD(smk_ipv6_port_list);
#ifdef CONFIG_SECURITY_SMACK_BRINGUP
@@ -2232,17 +2233,20 @@ static void smk_ipv6_port_label(struct socket
*sock, struct sockaddr *address)
* on the bound socket. Take the changes to the port
* as well.
*/
- list_for_each_entry(spp, &smk_ipv6_port_list, list) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) {
if (sk != spp->smk_sock)
continue;
spp->smk_in = ssp->smk_in;
spp->smk_out = ssp->smk_out;
+ rcu_read_unlock();
return;
}
/*
* A NULL address is only used for updating existing
* bound entries. If there isn't one, it's OK.
*/
+ rcu_read_unlock();
return;
}
@@ -2258,16 +2262,18 @@ static void smk_ipv6_port_label(struct socket
*sock, struct sockaddr *address)
* Look for an existing port list entry.
* This is an indication that a port is getting reused.
*/
- list_for_each_entry(spp, &smk_ipv6_port_list, list) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) {
if (spp->smk_port != port)
continue;
spp->smk_port = port;
spp->smk_sock = sk;
spp->smk_in = ssp->smk_in;
spp->smk_out = ssp->smk_out;
+ rcu_read_unlock();
return;
}
-
+ rcu_read_unlock();
/*
* A new port entry is required.
*/
@@ -2280,7 +2286,9 @@ static void smk_ipv6_port_label(struct socket
*sock, struct sockaddr *address)
spp->smk_in = ssp->smk_in;
spp->smk_out = ssp->smk_out;
- list_add(&spp->list, &smk_ipv6_port_list);
+ mutex_lock(&smack_ipv6_lock);
+ list_add_rcu(&spp->list, &smk_ipv6_port_list);
+ mutex_unlock(&smack_ipv6_lock);
return;
}
@@ -2335,8 +2343,8 @@ static int smk_ipv6_port_check(struct sock *sk,
struct sockaddr_in6 *address,
skp = &smack_known_web;
goto auditout;
}
-
- list_for_each_entry(spp, &smk_ipv6_port_list, list) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) {
if (spp->smk_port != port)
continue;
object = spp->smk_in;
@@ -2344,6 +2352,7 @@ static int smk_ipv6_port_check(struct sock *sk,
struct sockaddr_in6 *address,
ssp->smk_packet = spp->smk_out;
break;
}
+ rcu_read_unlock();
auditout:
--
1.8.3.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: Fix for synchronization issue in IPV6 implementation in smack module(v3.18) 2015-01-08 16:41 Fix for synchronization issue in IPV6 implementation in smack module(v3.18) Vishal Goel @ 2015-01-09 0:09 ` Casey Schaufler 2015-01-09 13:42 ` Ahmed S. Darwish 1 sibling, 0 replies; 4+ messages in thread From: Casey Schaufler @ 2015-01-09 0:09 UTC (permalink / raw) To: Vishal Goel, linux-kernel, linux-security-module; +Cc: Casey Schaufler On 1/8/2015 8:41 AM, Vishal Goel wrote: > [PATCH] This patch fixes the synchronization issue in IPv6 > implementation. Previously there was no synchronization mechanism used while > accessing(adding/reading/deletion) smk_ipv6_port_list. It could be possible > that when one thread is reading the list, at the same time another thread is > adding/deleting in the list.So it is possible that reader thread will read > the inaccurate or incomplete list. So to make sure that reader thread will > read the accurate list, rcu mechanism has been used while accessing the > list.RCU allows readers to access a data structure even when it is in the > process of being updated > > Signed-off-by: Vishal Goel <vishal.goel@samsung.com> > Himanshu Shukla <himanshu.sh@samsung.com> Your three patches are whitespace mangled. They also need to be rebased on my current tree: git://git.gitorious.org/smack-next/kernel.git using the branch smack-for-3.20. They look OK, but I can't apply them as they are. Thank you. > --- > security/smack/smack_lsm.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index d515ec2..b3427ee 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -52,6 +52,7 @@ > #define SMK_RECEIVING 1 > #define SMK_SENDING 2 > > +DEFINE_MUTEX(smack_ipv6_lock); > LIST_HEAD(smk_ipv6_port_list); > > #ifdef CONFIG_SECURITY_SMACK_BRINGUP > @@ -2232,17 +2233,20 @@ static void smk_ipv6_port_label(struct socket > *sock, struct sockaddr *address) > * on the bound socket. Take the changes to the port > * as well. > */ > - list_for_each_entry(spp, &smk_ipv6_port_list, list) { > + rcu_read_lock(); > + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { > if (sk != spp->smk_sock) > continue; > spp->smk_in = ssp->smk_in; > spp->smk_out = ssp->smk_out; > + rcu_read_unlock(); > return; > } > /* > * A NULL address is only used for updating existing > * bound entries. If there isn't one, it's OK. > */ > + rcu_read_unlock(); > return; > } > > @@ -2258,16 +2262,18 @@ static void smk_ipv6_port_label(struct socket > *sock, struct sockaddr *address) > * Look for an existing port list entry. > * This is an indication that a port is getting reused. > */ > - list_for_each_entry(spp, &smk_ipv6_port_list, list) { > + rcu_read_lock(); > + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { > if (spp->smk_port != port) > continue; > spp->smk_port = port; > spp->smk_sock = sk; > spp->smk_in = ssp->smk_in; > spp->smk_out = ssp->smk_out; > + rcu_read_unlock(); > return; > } > - > + rcu_read_unlock(); > /* > * A new port entry is required. > */ > @@ -2280,7 +2286,9 @@ static void smk_ipv6_port_label(struct socket > *sock, struct sockaddr *address) > spp->smk_in = ssp->smk_in; > spp->smk_out = ssp->smk_out; > > - list_add(&spp->list, &smk_ipv6_port_list); > + mutex_lock(&smack_ipv6_lock); > + list_add_rcu(&spp->list, &smk_ipv6_port_list); > + mutex_unlock(&smack_ipv6_lock); > return; > } > > @@ -2335,8 +2343,8 @@ static int smk_ipv6_port_check(struct sock *sk, > struct sockaddr_in6 *address, > skp = &smack_known_web; > goto auditout; > } > - > - list_for_each_entry(spp, &smk_ipv6_port_list, list) { > + rcu_read_lock(); > + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { > if (spp->smk_port != port) > continue; > object = spp->smk_in; > @@ -2344,6 +2352,7 @@ static int smk_ipv6_port_check(struct sock *sk, > struct sockaddr_in6 *address, > ssp->smk_packet = spp->smk_out; > break; > } > + rcu_read_unlock(); > > auditout: > > -- > 1.8.3.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fix for synchronization issue in IPV6 implementation in smack module(v3.18) 2015-01-08 16:41 Fix for synchronization issue in IPV6 implementation in smack module(v3.18) Vishal Goel 2015-01-09 0:09 ` Casey Schaufler @ 2015-01-09 13:42 ` Ahmed S. Darwish 2015-01-09 17:44 ` Casey Schaufler 1 sibling, 1 reply; 4+ messages in thread From: Ahmed S. Darwish @ 2015-01-09 13:42 UTC (permalink / raw) To: Vishal Goel; +Cc: linux-kernel, linux-security-module, casey Hi Vishal, On Thu, Jan 08, 2015 at 10:11:52PM +0530, Vishal Goel wrote: > [PATCH] This patch fixes the synchronization issue in IPv6 > implementation. Previously there was no synchronization mechanism used while > accessing(adding/reading/deletion) smk_ipv6_port_list. It could be possible > that when one thread is reading the list, at the same time another thread is > adding/deleting in the list.So it is possible that reader thread will read > the inaccurate or incomplete list. So to make sure that reader thread will > read the accurate list, rcu mechanism has been used while accessing the > list.RCU allows readers to access a data structure even when it is in the > process of being updated > > Signed-off-by: Vishal Goel <vishal.goel@samsung.com> > Himanshu Shukla <himanshu.sh@samsung.com> The legality of your patches are blurry. You're sending from a personal email, while having Signed-off-by signatures by your employer. You **really** need to add a "From: XXX@samsung.com" header on the very first line of your emails if this is a sponsored work. Kindly check Documentation/SubmittingPatches for further details. Beside the above: - Your patches are not applicable to the tree since they're white-spaces mangled. You're using Gmail's web interface, which is well known at converting tabs to white-spaces. Check Documentation/email-clients.txt for further details. - Please fix you Subject line. Make it something in the form of: [PATCH 1/3] smack: Fix xxx - No need for "[PATCH]" in the commit log body, only in the subject line. - Please make the commit message more comprehensible. Check the kernel git log history for good examples. A grammar check will also be nice; there are a number of free good tools on the web. - Add "Signed-off-by" headers for each developer. In the patch above, you'll need _two_ "Signed-off-by" lines. - You're sending multiple related patches, but posting each one in its own thread. This will make it very very hard for review, especially in a very busy list like LKML. Please send related patches in an "email thread", with clear sequence numbers. (e.g., your follow-up patch titled as "In Ref to previous 3 patches:Fix for synchronization..." is completely bogus.) Happy kernel coding :-) Regards, Darwish > --- > security/smack/smack_lsm.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index d515ec2..b3427ee 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -52,6 +52,7 @@ > #define SMK_RECEIVING 1 > #define SMK_SENDING 2 > > +DEFINE_MUTEX(smack_ipv6_lock); > LIST_HEAD(smk_ipv6_port_list); > > #ifdef CONFIG_SECURITY_SMACK_BRINGUP > @@ -2232,17 +2233,20 @@ static void smk_ipv6_port_label(struct socket > *sock, struct sockaddr *address) > * on the bound socket. Take the changes to the port > * as well. > */ > - list_for_each_entry(spp, &smk_ipv6_port_list, list) { > + rcu_read_lock(); > + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { > if (sk != spp->smk_sock) > continue; > spp->smk_in = ssp->smk_in; > spp->smk_out = ssp->smk_out; > + rcu_read_unlock(); > return; > } > /* > * A NULL address is only used for updating existing > * bound entries. If there isn't one, it's OK. > */ > + rcu_read_unlock(); > return; > } > > @@ -2258,16 +2262,18 @@ static void smk_ipv6_port_label(struct socket > *sock, struct sockaddr *address) > * Look for an existing port list entry. > * This is an indication that a port is getting reused. > */ > - list_for_each_entry(spp, &smk_ipv6_port_list, list) { > + rcu_read_lock(); > + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { > if (spp->smk_port != port) > continue; > spp->smk_port = port; > spp->smk_sock = sk; > spp->smk_in = ssp->smk_in; > spp->smk_out = ssp->smk_out; > + rcu_read_unlock(); > return; > } > - > + rcu_read_unlock(); > /* > * A new port entry is required. > */ > @@ -2280,7 +2286,9 @@ static void smk_ipv6_port_label(struct socket > *sock, struct sockaddr *address) > spp->smk_in = ssp->smk_in; > spp->smk_out = ssp->smk_out; > > - list_add(&spp->list, &smk_ipv6_port_list); > + mutex_lock(&smack_ipv6_lock); > + list_add_rcu(&spp->list, &smk_ipv6_port_list); > + mutex_unlock(&smack_ipv6_lock); > return; > } > > @@ -2335,8 +2343,8 @@ static int smk_ipv6_port_check(struct sock *sk, > struct sockaddr_in6 *address, > skp = &smack_known_web; > goto auditout; > } > - > - list_for_each_entry(spp, &smk_ipv6_port_list, list) { > + rcu_read_lock(); > + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { > if (spp->smk_port != port) > continue; > object = spp->smk_in; > @@ -2344,6 +2352,7 @@ static int smk_ipv6_port_check(struct sock *sk, > struct sockaddr_in6 *address, > ssp->smk_packet = spp->smk_out; > break; > } > + rcu_read_unlock(); > > auditout: > > -- > 1.8.3.2 > -- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fix for synchronization issue in IPV6 implementation in smack module(v3.18) 2015-01-09 13:42 ` Ahmed S. Darwish @ 2015-01-09 17:44 ` Casey Schaufler 0 siblings, 0 replies; 4+ messages in thread From: Casey Schaufler @ 2015-01-09 17:44 UTC (permalink / raw) To: Ahmed S. Darwish, Vishal Goel Cc: linux-kernel, linux-security-module, Casey Schaufler On 1/9/2015 5:42 AM, Ahmed S. Darwish wrote: > Hi Vishal, > > On Thu, Jan 08, 2015 at 10:11:52PM +0530, Vishal Goel wrote: >> [PATCH] This patch fixes the synchronization issue in IPv6 >> implementation. Previously there was no synchronization mechanism used while >> accessing(adding/reading/deletion) smk_ipv6_port_list. It could be possible >> that when one thread is reading the list, at the same time another thread is >> adding/deleting in the list.So it is possible that reader thread will read >> the inaccurate or incomplete list. So to make sure that reader thread will >> read the accurate list, rcu mechanism has been used while accessing the >> list.RCU allows readers to access a data structure even when it is in the >> process of being updated >> >> Signed-off-by: Vishal Goel <vishal.goel@samsung.com> >> Himanshu Shukla <himanshu.sh@samsung.com> > The legality of your patches are blurry. You're sending from > a personal email, while having Signed-off-by signatures by your > employer. > > You **really** need to add a "From: XXX@samsung.com" header on > the very first line of your emails if this is a sponsored work. > Kindly check Documentation/SubmittingPatches for further details. > > Beside the above: > > - Your patches are not applicable to the tree since they're > white-spaces mangled. You're using Gmail's web interface, which > is well known at converting tabs to white-spaces. Check > Documentation/email-clients.txt for further details. > > - Please fix you Subject line. Make it something in the form of: > [PATCH 1/3] smack: Fix xxx > > - No need for "[PATCH]" in the commit log body, only in the > subject line. > > - Please make the commit message more comprehensible. Check > the kernel git log history for good examples. A grammar > check will also be nice; there are a number of free good > tools on the web. > > - Add "Signed-off-by" headers for each developer. In the patch > above, you'll need _two_ "Signed-off-by" lines. > > - You're sending multiple related patches, but posting each one > in its own thread. This will make it very very hard for review, > especially in a very busy list like LKML. Please send related > patches in an "email thread", with clear sequence numbers. > > (e.g., your follow-up patch titled as "In Ref to previous 3 > patches:Fix for synchronization..." is completely bogus.) > > Happy kernel coding :-) > > Regards, > Darwish Further, they still are based on the wrong tree. These patches need to be based on the smack-next tree: git://git.gitorious.org/smack-next/kernel.git branch smack-for-3.20 There has been other work on the IPv6 code for 3.20. Your patches, even when demangled, do not apply. > >> --- >> security/smack/smack_lsm.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index d515ec2..b3427ee 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -52,6 +52,7 @@ >> #define SMK_RECEIVING 1 >> #define SMK_SENDING 2 >> >> +DEFINE_MUTEX(smack_ipv6_lock); >> LIST_HEAD(smk_ipv6_port_list); >> >> #ifdef CONFIG_SECURITY_SMACK_BRINGUP >> @@ -2232,17 +2233,20 @@ static void smk_ipv6_port_label(struct socket >> *sock, struct sockaddr *address) >> * on the bound socket. Take the changes to the port >> * as well. >> */ >> - list_for_each_entry(spp, &smk_ipv6_port_list, list) { >> + rcu_read_lock(); >> + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { >> if (sk != spp->smk_sock) >> continue; >> spp->smk_in = ssp->smk_in; >> spp->smk_out = ssp->smk_out; >> + rcu_read_unlock(); >> return; >> } >> /* >> * A NULL address is only used for updating existing >> * bound entries. If there isn't one, it's OK. >> */ >> + rcu_read_unlock(); >> return; >> } >> >> @@ -2258,16 +2262,18 @@ static void smk_ipv6_port_label(struct socket >> *sock, struct sockaddr *address) >> * Look for an existing port list entry. >> * This is an indication that a port is getting reused. >> */ >> - list_for_each_entry(spp, &smk_ipv6_port_list, list) { >> + rcu_read_lock(); >> + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { >> if (spp->smk_port != port) >> continue; >> spp->smk_port = port; >> spp->smk_sock = sk; >> spp->smk_in = ssp->smk_in; >> spp->smk_out = ssp->smk_out; >> + rcu_read_unlock(); >> return; >> } >> - >> + rcu_read_unlock(); >> /* >> * A new port entry is required. >> */ >> @@ -2280,7 +2286,9 @@ static void smk_ipv6_port_label(struct socket >> *sock, struct sockaddr *address) >> spp->smk_in = ssp->smk_in; >> spp->smk_out = ssp->smk_out; >> >> - list_add(&spp->list, &smk_ipv6_port_list); >> + mutex_lock(&smack_ipv6_lock); >> + list_add_rcu(&spp->list, &smk_ipv6_port_list); >> + mutex_unlock(&smack_ipv6_lock); >> return; >> } >> >> @@ -2335,8 +2343,8 @@ static int smk_ipv6_port_check(struct sock *sk, >> struct sockaddr_in6 *address, >> skp = &smack_known_web; >> goto auditout; >> } >> - >> - list_for_each_entry(spp, &smk_ipv6_port_list, list) { >> + rcu_read_lock(); >> + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { >> if (spp->smk_port != port) >> continue; >> object = spp->smk_in; >> @@ -2344,6 +2352,7 @@ static int smk_ipv6_port_check(struct sock *sk, >> struct sockaddr_in6 *address, >> ssp->smk_packet = spp->smk_out; >> break; >> } >> + rcu_read_unlock(); >> >> auditout: >> >> -- >> 1.8.3.2 >> -- > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-01-09 17:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-08 16:41 Fix for synchronization issue in IPV6 implementation in smack module(v3.18) Vishal Goel 2015-01-09 0:09 ` Casey Schaufler 2015-01-09 13:42 ` Ahmed S. Darwish 2015-01-09 17:44 ` Casey Schaufler
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox