Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xia/lpm: remove routing/forwarding deadlock #17

Open
wants to merge 2 commits into
base: xia
Choose a base branch
from
Open

xia/lpm: remove routing/forwarding deadlock #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d895df
xia/lpm: remove routing/forwarding deadlock
cjdoucette May 16, 2016
407b4ff
xia: add fxid_copy to FIB interface
cjdoucette Sep 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions include/net/xia_fib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,16 @@ struct xia_ppal_rt_iops {
*/
void (*fxid_init)(struct fib_xid *fxid, int table_id, int entry_type);

/* fxid_copy - copy @old_fxid, including FIB-specific data.
*
* NOTE
* This function must copy any FIB-specific data in addition
* to the fields from struct fib_xid. @old_fxid and @new_fxid
* should reference FIB entries previously allocated by
* fxid_ppal_alloc().
*/
void (*fxid_copy)(struct fib_xid *old_fxid, struct fib_xid *new_fxid);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind swapping the position of the parameters? The assigning order is followed by the C library, and it bothers to go against it. A few examples: memcpy(), memmove(), and strcpy().


/** fxid_find_rcu - Find struct fib_xid in @xtbl that has key @xid.
*
* RETURN
Expand Down
7 changes: 7 additions & 0 deletions net/xia/list_fib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,12 @@ static void list_fxid_init(struct fib_xid *fxid, int table_id, int entry_type)
fxid->dead.xtbl = NULL;
}

static void list_fxid_copy(struct fib_xid *old_fxid, struct fib_xid *new_fxid)
{
memcpy(new_fxid, old_fxid,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it's unlikely that new_fxid == old_fxid, it would be safer to replace memcpy() with memmove().

sizeof(*old_fxid) + sizeof(struct list_fib_xid));
}

static void list_xtbl_death_work(struct work_struct *work)
{
struct fib_xid_table *xtbl = container_of(work, struct fib_xid_table,
Expand Down Expand Up @@ -621,6 +627,7 @@ const struct xia_ppal_rt_iops xia_ppal_list_rt_iops = {

.fxid_ppal_alloc = list_fxid_ppal_alloc,
.fxid_init = list_fxid_init,
.fxid_copy = list_fxid_copy,

.fxid_find_rcu = list_fxid_find_rcu,
.fxid_find_lock = list_fxid_find_lock,
Expand Down
150 changes: 116 additions & 34 deletions net/xia/ppal_lpm/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,36 +46,113 @@ static inline struct fib_xid_lpm_local *fxid_llpm(struct fib_xid *fxid)
*/
const struct xia_ppal_rt_iops *lpm_rt_iops = &xia_ppal_tree_rt_iops;

/* Only call this function after an RCU synchronization,
* such as by calling free_fxid.
*/
static void local_free_lpm(struct fib_xid_table *xtbl, struct fib_xid *fxid)
{
struct fib_xid_lpm_local *llpm = fxid_llpm(fxid);

xdst_free_anchor(&llpm->anchor);
kfree(llpm);
}

/* Assuming the FIB is locked, find the appropriate anchor,
* flush it, and unlock the FIB.
* flush it, and unlock the FIB. To do this, we create a copy of
* the predecessor so that some readers can continue using the tree while
* we wait for other readers to finish to flush the anchor.
*
* NOTE
* In rare cases, this function can fail due to lack of memory,
* leaving the code unable to flush an anchor for a previous
* entry. In these cases, a new entry cannot be added, so it
* will be removed by this function and freed by the caller.
*/
static void newroute_flush_anchor_locked(struct fib_xid_table *xtbl,
struct fib_xid *new_fxid,
struct xip_deferred_negdep_flush *dnf)
static int newroute_flush_anchor_unlock(struct fib_xid_table *xtbl,
struct fib_xid *new_fxid,
struct xip_deferred_negdep_flush *dnf)
{
struct fib_xid *pred_fxid = tree_fib_get_pred_locked(new_fxid);
/* At most one of @dup_llpm and @dup_mrd should be used. */
struct fib_xid_lpm_local *dup_llpm = NULL;
struct fib_xid_redirect_main *dup_mrd = NULL;

/* Find the predecessor. If it doesn't exist, we're done. */
struct fib_xid *pred_fxid = tree_fib_get_pred_locked(new_fxid);
if (!pred_fxid) {
lpm_rt_iops->fib_unlock(xtbl, NULL);
fib_defer_dnf(dnf, xtbl_net(xtbl), xtbl_ppalty(xtbl));
return;
return 0;
}

fib_free_dnf(dnf);
synchronize_rcu();
/* Flush the predecessor's anchor by first making a copy,
* replacing the old entry, waiting an RCU synchronization,
* and then freeing the old entry when done.
*/
switch (pred_fxid->fx_table_id) {
case XRTABLE_LOCAL_INDEX:
xdst_free_anchor(&fxid_llpm(pred_fxid)->anchor);
/* Allocate a duplicate of the predecessor entry. */
dup_llpm = lpm_rt_iops->fxid_ppal_alloc(sizeof(*dup_llpm),
GFP_ATOMIC);
if (!dup_llpm) {
/* Can't add this entry now due to lack of memory. */
lpm_rt_iops->fxid_rm_locked(NULL, xtbl, new_fxid);
lpm_rt_iops->fib_unlock(xtbl, NULL);
return -ENOMEM;
}

/* Replace the old predecessor with the new predecessor by
* copying the generic struct fib_xid and replacing the old
* node with the new one in the tree.
*/
xdst_init_anchor(&dup_llpm->anchor);
lpm_rt_iops->fxid_copy(&dup_llpm->common, pred_fxid);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've inverted the parameters of fxid_copy() above, you fixed a bug here.

lpm_rt_iops->fxid_replace_locked(xtbl, pred_fxid,
&dup_llpm->common);

/* Release write lock to let tree readers that get a write
* lock (such as in lpm_deliver()) continue, avoiding deadlock.
*/
lpm_rt_iops->fib_unlock(xtbl, NULL);

/* Wait for existing RCU readers in routing mechanism to
* finish, and then flush the anchor.
*
* The old predecessor is no longer accessible by the tree and
* existing readers on its anchor have finished, so we can
* release the old predecessor. Since we just called
* synchronize_rcu(), we can directly call local_free_lpm().
*/
synchronize_rcu();
local_free_lpm(xtbl, pred_fxid);
BUG_ON(dup_mrd);
break;
case XRTABLE_MAIN_INDEX:
xdst_invalidate_redirect(xtbl_net(xtbl), xtbl_ppalty(xtbl),
pred_fxid->fx_xid,
&fxid_mrd(pred_fxid)->gw);
/* Same algorithm as above for main predecessor entries. */
dup_mrd = lpm_rt_iops->fxid_ppal_alloc(sizeof(*dup_mrd),
GFP_ATOMIC);
if (!dup_mrd) {
lpm_rt_iops->fxid_rm_locked(NULL, xtbl, new_fxid);
lpm_rt_iops->fib_unlock(xtbl, NULL);
return -ENOMEM;
}

dup_mrd->gw = fxid_mrd(pred_fxid)->gw;
lpm_rt_iops->fxid_copy(&dup_mrd->common, pred_fxid);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've inverted the parameters of fxid_copy() above, you fixed another bug here.

lpm_rt_iops->fxid_replace_locked(xtbl, pred_fxid,
&dup_mrd->common);

lpm_rt_iops->fib_unlock(xtbl, NULL);

synchronize_rcu();
fib_mrd_free(xtbl, pred_fxid);
BUG_ON(dup_llpm);
break;
default:
BUG();
}
lpm_rt_iops->fib_unlock(xtbl, NULL);

fib_free_dnf(dnf);
return 0;
}

static int local_newroute(struct xip_ppal_ctx *ctx,
Expand Down Expand Up @@ -111,15 +188,22 @@ static int local_newroute(struct xip_ppal_ctx *ctx,
* atomically to flush the appropriate anchor.
*/
rc = tree_fib_newroute_lock(&new_llpm->common, xtbl, cfg, NULL);
if (rc) {
fib_free_dnf(dnf);
fxid_free_norcu(xtbl, &new_llpm->common);
return rc;
}
if (rc)
goto unlock_and_free;

/* Flush appropriate anchor and release lock. */
newroute_flush_anchor_locked(xtbl, &new_llpm->common, dnf);
rc = newroute_flush_anchor_unlock(xtbl, &new_llpm->common, dnf);
if (rc)
goto free;

return 0;

unlock_and_free:
lpm_rt_iops->fib_unlock(NULL, xtbl);
free:
fib_free_dnf(dnf);
fxid_free_norcu(xtbl, &new_llpm->common);
return rc;
}

static int local_dump_lpm(struct fib_xid *fxid, struct fib_xid_table *xtbl,
Expand Down Expand Up @@ -169,15 +253,6 @@ static int local_dump_lpm(struct fib_xid *fxid, struct fib_xid_table *xtbl,
return -EMSGSIZE;
}

/* Don't call this function! Use free_fxid instead. */
static void local_free_lpm(struct fib_xid_table *xtbl, struct fib_xid *fxid)
{
struct fib_xid_lpm_local *llpm = fxid_llpm(fxid);

xdst_free_anchor(&llpm->anchor);
kfree(llpm);
}

static int main_newroute(struct xip_ppal_ctx *ctx, struct fib_xid_table *xtbl,
struct xia_fib_config *cfg)
{
Expand Down Expand Up @@ -208,15 +283,22 @@ static int main_newroute(struct xip_ppal_ctx *ctx, struct fib_xid_table *xtbl,
* atomically to flush the appropriate anchor.
*/
rc = tree_fib_newroute_lock(&new_mrd->common, xtbl, cfg, NULL);
if (rc) {
fib_free_dnf(dnf);
fxid_free_norcu(xtbl, &new_mrd->common);
return rc;
}
if (rc)
goto unlock_and_free;

/* Flush appropriate anchor and release lock. */
newroute_flush_anchor_locked(xtbl, &new_mrd->common, dnf);
rc = newroute_flush_anchor_unlock(xtbl, &new_mrd->common, dnf);
if (rc)
goto free;

return 0;

unlock_and_free:
lpm_rt_iops->fib_unlock(NULL, xtbl);
free:
fib_free_dnf(dnf);
fxid_free_norcu(xtbl, &new_mrd->common);
return rc;
}

static const xia_ppal_all_rt_eops_t lpm_all_rt_eops = {
Expand Down
7 changes: 7 additions & 0 deletions net/xia/ppal_lpm/tree_fib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ static void tree_fxid_init(struct fib_xid *fxid, int table_id, int entry_type)
fxid->dead.xtbl = NULL;
}

static void tree_fxid_copy(struct fib_xid *old_fxid, struct fib_xid *new_fxid)
{
memcpy(new_fxid, old_fxid,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it's unlikely that new_fxid == old_fxid, it would be safer to replace memcpy() with memmove().

sizeof(*old_fxid) + sizeof(struct tree_fib_xid));
}

static inline void disconnect_from_parent(struct tree_fib_xid *node)
{
if (node && node->parent) {
Expand Down Expand Up @@ -683,6 +689,7 @@ const struct xia_ppal_rt_iops xia_ppal_tree_rt_iops = {

.fxid_ppal_alloc = tree_fxid_ppal_alloc,
.fxid_init = tree_fxid_init,
.fxid_copy = tree_fxid_copy,

/* Note that there is no RCU-specific version. */
.fxid_find_rcu = tree_fxid_find_rcu,
Expand Down