Re: [Ocf-linux-users] patch for OCF
Brought to you by:
david-m
Menu
▾
▴
Re: [Ocf-linux-users] patch for OCF
|
From: hwang7 <hua...@wi...> - 2013-04-24 01:33:09
|
> hwang7 wrote the following:
>> Hi david
>>
>> When using cryptosoft of OCF to do the crypto and run test scripts
>> which includes the following commands on SMP platform(we do the test
>> on fsl_p4080 which is a SMP processor with 8 cores):
>>
>> $ openssl speed -evp des -engine cryptodev -elapsed -multi 10
>> $ openssl speed -evp des3 -engine cryptodev -elapsed -multi 10
>> $ openssl speed -evp aes128 -engine cryptodev -elapsed -multi 10
>>
>> The system will give out call trace or hang.
>>
>> The root causes of this issue is:
>> The swcr_sessions is not protected properly, and the three
>> types of session operation "create session, use session, delete
>> session" are also not protected properly.
>>
>> So:
>> 1.add spin lock to protect swcr_sessions
>> 2.add state/ref fields to session, use session state to prevent from
>> deleting a session when it's in creating or using state, and use spin
>> lock to protect the session state.
>
> Thanks for sending in the patch. I did see it the first time you posted it,
> I just haven't had time to digest it fully, sorry for not responding
> earlier.
>
> Does the fsl_p4080 have any HW acceleration underneath cryptosoft or are
> you using purely software acceleration ?
>
I use purely software acceleration to do the test.
> The patch looks pretty good, but it would help me a lot if you could
> describe what kind of re-entry/call sequence was causing the main problem.
>
For CIOCFSESSION the execute path is :
1) cse = csefind(fcr, ses);
2) judge if cse is NULL
3) csedelete(fcr, cse);
4) error = csefree(cse);
For step4, if there are two delete requesets to the same cse called
"delte_request1" and "delete_request2", then the swcr_freesession will
also be called two times. Then in the below condition, there will be
problem with cryptosoft:
1) delte_request1 and delete_request2 is sent to kernel, both of
these requests execute the csefind(fcr, ses) sucessfully
2) delte_request1 is finished, so the slot for the session in
swcr_sessions is free
3) a create session is send to kernel, and it get the slot who is
free at step 2)
4) create session requeset is processed, and then wait for some
resources
5) delte_request2 get the cpu, and so it will delete the session
created in step4
6) create session requeset is wake up, and it will work on a
session who is not exist...
Below is a log for this issue( this log is got from another SMP platform
P2041):
Unable to handle kernel paging request for data at address 0x00000008
Faulting instruction address: 0xfa29abf4
Oops: Kernel access of bad area, sig: 11 [#1]
PREEMPT SMP NR_CPUS=4 LTT NESTING LEVEL : 0
P2041 RDB
last sysfs file: /sys/devices/virtual/misc/crypto/uevent
Modules linked in: ipv6 sctp binfmt_misc bsdjail exportfs xfs jfs
reiserfs jbd2
ext4 squashfs isofs cramfs jffs2 yaffs fat vfat msdos nls_cp437 ocf(P)
cryptodev
(P) cryptosoft aes_generic sha1_generic sha256_generic ctr michael_mic
xcbc blow
fish cast5 crypto_null sha512_generic camellia [last unloaded:
xfrm6_mode_transp
ort]
NIP: fa29abf4 LR: fa29abec CTR: c0678348
REGS: eb1d9b80 TRAP: 0300 Tainted: P
(2.6.34.10-WR4.3.0.0_standard)
MSR: 00029002 <EE,ME,CE> CR: 24244448 XER: 20000000
DEAR: 00000008, ESR: 00000000
TASK = eaa25700[3776] 'openssl' THREAD: eb1d8000 CPU: 3
GPR00: fa29abec eb1d9c30 eaa25700 ea702cc0 c015012c e946d7ac 00000001
eb1d9ab0
GPR08: 077af000 eb1dc540 00000000 eb1d9c30 24244448 10081f08 fa29b700
eb1d8000
GPR16: fa29c45c fa29c460 00000000 fa29c0b0 c08cdac0 fa29b6a0 c08cdd7c
fa29c464
GPR24: c08c2f60 fa2a0000 00000000 fa29c3f8 00000001 eb1d9d48 e64e2484
eb1d9c30
NIP [fa29abf4] swcr_newsession+0x5fc/0xad4 [cryptosoft]
LR [fa29abec] swcr_newsession+0x5f4/0xad4 [cryptosoft]
Call Trace:
[eb1d9c30] [fa29abec] swcr_newsession+0x5f4/0xad4 [cryptosoft] (unreliable)
[eb1d9c90] [fa26fbf4] crypto_newsession+0x12c/0x36c [ocf]
[eb1d9cd0] [fa288164] cryptodev_ioctl+0xc68/0x20b8 [cryptodev]
[eb1d9e60] [fa2895ec] cryptodev_unlocked_ioctl+0x38/0x50 [cryptodev]
[eb1d9e70] [c0166370] vfs_ioctl+0x4c/0xf8
[eb1d9e90] [c016660c] do_vfs_ioctl+0x90/0x760
[eb1d9f00] [c0166d70] sys_ioctl+0x94/0x124
[eb1d9f40] [c0012970] ret_from_syscall+0x0/0x4
> Thanks,
> Davidm
>
>
>
>
Best Regards
========================
Wang Huanhuan
Work phone: +86-10-8477-8568
>> diff -urbN ocf-linux-20120127/ocf/cryptosoft.c ocf-linux-20120127-1/ocf/cryptosoft.c
>> --- ocf-linux-20120127/ocf/cryptosoft.c 2012-01-27 09:04:28.000000000 +0800
>> +++ ocf-linux-20120127-1/ocf/cryptosoft.c 2013-01-30 12:41:25.554624164 +0800
>> @@ -77,6 +77,11 @@
>>
>> #define SW_TYPE_INUSE 0x10000000
>>
>> +#define SW_SESSION_IDLE 0x0
>> +#define SW_SESSION_CREATION_IN_PROGRESS 0x1
>> +#define SW_SESSION_IN_USE 0x2
>> +#define SW_SESSION_DELETION_IN_PROGRESS 0x3
>> +
>> /* We change some of the above if we have an async interface */
>>
>> #define SW_TYPE_ALG_AMASK (SW_TYPE_ALG_MASK | SW_TYPE_ASYNC)
>> @@ -91,6 +96,8 @@
>> struct work_struct workq;
>> int sw_type;
>> int sw_alg;
>> + int sw_state;
>> + int sw_ref;
>> struct crypto_tfm *sw_tfm;
>> spinlock_t sw_tfm_lock;
>> union {
>> @@ -283,6 +290,25 @@
>> MODULE_PARM_DESC(swcr_debug, "Enable debug");
>>
>> static void swcr_process_req(struct swcr_req *req);
>> +static DEFINE_SPINLOCK(session_lock);
>> +
>> +static inline void _swcr_set_state(struct swcr_data *swd, int state, int lock)
>> +{
>> + unsigned long flags;
>> + if( lock )
>> + spin_lock_irqsave(&session_lock, flags);
>> +
>> + if (swd != NULL)
>> + swd->sw_state = state;
>> +
>> + if ( lock )
>> + spin_unlock_irqrestore(&session_lock, flags);
>> +}
>> +
>> +static inline void swcr_set_state(struct swcr_data *swd, int state)
>> +{
>> + _swcr_set_state(swd, state, 0);
>> +}
>>
>> /*
>> * somethings just need to be run with user context no matter whether
>> @@ -339,14 +365,17 @@
>> swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
>> {
>> struct swcr_data **swd;
>> - u_int32_t i;
>> + u_int32_t index, i;
>> int error;
>> char *algo;
>> int mode;
>> + unsigned long flags;
>>
>> + spin_lock_irqsave(&session_lock, flags);
>> dprintk("%s()\n", __FUNCTION__);
>> if (sid == NULL || cri == NULL) {
>> dprintk("%s,%d - EINVAL\n", __FILE__, __LINE__);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> return EINVAL;
>> }
>>
>> @@ -372,6 +401,7 @@
>> else
>> swcr_sesnum /= 2;
>> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> return ENOBUFS;
>> }
>> memset(swd, 0, swcr_sesnum * sizeof(struct swcr_data *));
>> @@ -386,30 +416,39 @@
>> swcr_sessions = swd;
>> }
>>
>> - swd = &swcr_sessions[i];
>> - *sid = i;
>> + index = i;
>> + swd = &swcr_sessions[index];
>> + *sid = index;
>>
>> while (cri) {
>> *swd = (struct swcr_data *) kmalloc(sizeof(struct swcr_data),
>> SLAB_ATOMIC);
>> if (*swd == NULL) {
>> - swcr_freesession(NULL, i);
>> + swcr_set_state(swcr_sessions[index], SW_SESSION_DELETION_IN_PROGRESS);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> + swcr_freesession(NULL, index);
>> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
>> return ENOBUFS;
>> }
>> memset(*swd, 0, sizeof(struct swcr_data));
>> + swcr_set_state(*swd, SW_SESSION_CREATION_IN_PROGRESS);
>> + spin_unlock_irqrestore(&session_lock, flags);
>>
>> if (cri->cri_alg < 0 ||
>> cri->cri_alg>=sizeof(crypto_details)/sizeof(crypto_details[0])){
>> printk("cryptosoft: Unknown algorithm 0x%x\n", cri->cri_alg);
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> return EINVAL;
>> }
>>
>> algo = crypto_details[cri->cri_alg].alg_name;
>> if (!algo || !*algo) {
>> printk("cryptosoft: Unsupported algorithm 0x%x\n", cri->cri_alg);
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> return EINVAL;
>> }
>>
>> @@ -450,7 +489,9 @@
>> algo,mode);
>> err = IS_ERR((*swd)->sw_tfm) ? -(PTR_ERR((*swd)->sw_tfm)) : EINVAL;
>> (*swd)->sw_tfm = NULL; /* ensure NULL */
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> return err;
>> }
>>
>> @@ -482,7 +523,9 @@
>> if (error) {
>> printk("cryptosoft: setkey failed %d (crt_flags=0x%x)\n", error,
>> (*swd)->sw_tfm->crt_flags);
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> return error;
>> }
>> } else if ((*swd)->sw_type & (SW_TYPE_HMAC | SW_TYPE_HASH)) {
>> @@ -504,7 +547,9 @@
>> if (!(*swd)->sw_tfm) {
>> dprintk("cryptosoft: crypto_alloc_hash failed(%s,0x%x)\n",
>> algo, mode);
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> return EINVAL;
>> }
>>
>> @@ -512,7 +557,9 @@
>> (*swd)->u.hmac.sw_key = (char *)kmalloc((*swd)->u.hmac.sw_klen,
>> SLAB_ATOMIC);
>> if ((*swd)->u.hmac.sw_key == NULL) {
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
>> return ENOBUFS;
>> }
>> @@ -532,24 +579,33 @@
>> if (!(*swd)->sw_tfm) {
>> dprintk("cryptosoft: crypto_alloc_comp failed(%s,0x%x)\n",
>> algo, mode);
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> return EINVAL;
>> }
>> (*swd)->u.sw_comp_buf = kmalloc(CRYPTO_MAX_DATA_LEN, SLAB_ATOMIC);
>> if ((*swd)->u.sw_comp_buf == NULL) {
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
>> return ENOBUFS;
>> }
>> } else {
>> printk("cryptosoft: Unhandled sw_type %d\n", (*swd)->sw_type);
>> - swcr_freesession(NULL, i);
>> + _swcr_set_state(swcr_sessions[index],
>> + SW_SESSION_DELETION_IN_PROGRESS, 1);
>> + swcr_freesession(NULL, index);
>> return EINVAL;
>> }
>>
>> cri = cri->cri_next;
>> swd = &((*swd)->sw_next);
>> + spin_lock_irqsave(&session_lock, flags);
>> }
>> + swcr_set_state(swcr_sessions[index], SW_SESSION_IDLE);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> return 0;
>> }
>>
>> @@ -559,22 +615,42 @@
>> static int
>> swcr_freesession(device_t dev, u_int64_t tid)
>> {
>> + unsigned long flags;
>> struct swcr_data *swd;
>> u_int32_t sid = CRYPTO_SESID2LID(tid);
>>
>> + spin_lock_irqsave(&session_lock, flags);
>> dprintk("%s()\n", __FUNCTION__);
>> if (sid > swcr_sesnum || swcr_sessions == NULL ||
>> swcr_sessions[sid] == NULL) {
>> dprintk("%s,%d: EINVAL\n", __FILE__, __LINE__);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> return(EINVAL);
>> }
>>
>> /* Silently accept and return */
>> - if (sid == 0)
>> + if (sid == 0) {
>> + spin_unlock_irqrestore(&session_lock, flags);
>> return(0);
>> + }
>>
>> while ((swd = swcr_sessions[sid]) != NULL) {
>> + if (swd->sw_state == SW_SESSION_IN_USE
>> + || swd->sw_state == SW_SESSION_CREATION_IN_PROGRESS) {
>> + spin_unlock_irqrestore(&session_lock, flags);
>> + return EAGAIN;
>> + }
>> + /*
>> + * If the session is to be deleted, it should all be deleted
>> + * so mark it as unuseable.
>> + * In fact, only the first swcr_data's state is used as the
>> + * session state now, so just change it to mark the session
>> + * as being delete
>> + */
>> + swcr_set_state(swd, SW_SESSION_DELETION_IN_PROGRESS);
>> swcr_sessions[sid] = swd->sw_next;
>> + swcr_set_state(swcr_sessions[sid], SW_SESSION_DELETION_IN_PROGRESS);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> if (swd->sw_tfm) {
>> switch (swd->sw_type & SW_TYPE_ALG_AMASK) {
>> #ifdef HAVE_AHASH
>> @@ -615,16 +691,18 @@
>> kfree(swd->u.hmac.sw_key);
>> }
>> kfree(swd);
>> + spin_lock_irqsave(&session_lock, flags);
>> }
>> + spin_unlock_irqrestore(&session_lock, flags);
>> return 0;
>> }
>>
>> static void swcr_process_req_complete(struct swcr_req *req)
>> {
>> + unsigned long flags;
>> dprintk("%s()\n", __FUNCTION__);
>>
>> if (req->sw->sw_type & SW_TYPE_INUSE) {
>> - unsigned long flags;
>> spin_lock_irqsave(&req->sw->sw_tfm_lock, flags);
>> req->sw->sw_type &= ~SW_TYPE_INUSE;
>> spin_unlock_irqrestore(&req->sw->sw_tfm_lock, flags);
>> @@ -665,6 +743,12 @@
>> }
>>
>> done:
>> + spin_lock_irqsave(&session_lock, flags);
>> + req->sw_head->sw_ref--;
>> + if (req->sw_head->sw_ref <= 0) {
>> + swcr_set_state(req->sw_head, SW_SESSION_IDLE);
>> + }
>> + spin_unlock_irqrestore(&session_lock, flags);
>> dprintk("%s crypto_done %p\n", __FUNCTION__, req);
>> crypto_done(req->crp);
>> kmem_cache_free(swcr_req_cache, req);
>> @@ -1162,6 +1246,7 @@
>> {
>> struct swcr_req *req = NULL;
>> u_int32_t lid;
>> + unsigned long flags;
>>
>> dprintk("%s()\n", __FUNCTION__);
>> /* Sanity check */
>> @@ -1179,10 +1264,12 @@
>> }
>>
>> lid = crp->crp_sid & 0xffffffff;
>> + spin_lock_irqsave(&session_lock, flags);
>> if (lid >= swcr_sesnum || lid == 0 || swcr_sessions == NULL ||
>> swcr_sessions[lid] == NULL) {
>> crp->crp_etype = ENOENT;
>> dprintk("%s,%d: ENOENT\n", __FILE__, __LINE__);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> goto done;
>> }
>>
>> @@ -1195,6 +1282,7 @@
>> if (skb_shinfo(skb)->nr_frags >= SCATTERLIST_MAX) {
>> printk("%s,%d: %d nr_frags > SCATTERLIST_MAX", __FILE__, __LINE__,
>> skb_shinfo(skb)->nr_frags);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> goto done;
>> }
>> } else if (crp->crp_flags & CRYPTO_F_IOV) {
>> @@ -1202,6 +1290,7 @@
>> if (uiop->uio_iovcnt > SCATTERLIST_MAX) {
>> printk("%s,%d: %d uio_iovcnt > SCATTERLIST_MAX", __FILE__, __LINE__,
>> uiop->uio_iovcnt);
>> + spin_unlock_irqrestore(&session_lock, flags);
>> goto done;
>> }
>> }
>> @@ -1213,11 +1302,24 @@
>> if (req == NULL) {
>> dprintk("%s,%d: ENOMEM\n", __FILE__, __LINE__);
>> crp->crp_etype = ENOMEM;
>> + spin_unlock_irqrestore(&session_lock, flags);
>> goto done;
>> }
>> memset(req, 0, sizeof(*req));
>>
>> req->sw_head = swcr_sessions[lid];
>> + if (req->sw_head->sw_state == SW_SESSION_DELETION_IN_PROGRESS) {
>> + crp->crp_etype = EINVAL;
>> + spin_unlock_irqrestore(&session_lock, flags);
>> + goto done;
>> + } else if (req->sw_head->sw_state == SW_SESSION_CREATION_IN_PROGRESS) {
>> + crp->crp_etype = EAGAIN;
>> + spin_unlock_irqrestore(&session_lock, flags);
>> + goto done;
>> + }
>> + swcr_set_state(req->sw_head, SW_SESSION_IN_USE);
>> + req->sw_head->sw_ref++;
>> + spin_unlock_irqrestore(&session_lock, flags);
>> req->crp = crp;
>> req->crd = crp->crp_desc;
>>
>>
>
>> ------------------------------------------------------------------------------
>> Try New Relic Now & We'll Send You this Cool Shirt
>> New Relic is the only SaaS-based application performance monitoring service
>> that delivers powerful full stack analytics. Optimize and monitor your
>> browser, app, & servers with just a few lines of code. Try New Relic
>> and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr
>
>> _______________________________________________
>> Ocf-linux-users mailing list
>> Ocf...@li...
>> https://lists.sourceforge.net/lists/listinfo/ocf-linux-users
>
>
--
|
